Bug 73310

Summary: unicode-bidi:plaintext is supposed to be effective on display:inline elements too
Product: WebKit Reporter: Aharon (Vladimir) Lanin <aharon>
Component: CSSAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: aharon, eric, leviw, mitz, playmobil, rniwa, webkit.review.bot, xji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 69267, 78092    
Bug Blocks: 50910    
Attachments:
Description Flags
Test case (although should really be converted to a ref test)
none
Work in progress
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Aharon (Vladimir) Lanin
Reported 2011-11-29 04:53:23 PST
At the time that https://bugs.webkit.org/show_bug.cgi?id=50949 was filed and implementation of unicode-bidi:plaintext was begun, unicode-bidi:isolate was supposed to have "no effect on inline elements" (http://www.w3.org/TR/2011/WD-css3-writing-modes-20110428/). Subsequently, this changed (although I unfortunately only became aware of this today). From http://www.w3.org/TR/2011/WD-css3-writing-modes-20110901/ onward, the spec says: "For inline elements, this value behaves as for ‘isolate’, except, as with block containers, the base directionality is determined by following the Unicode heuristic instead of by using the ‘direction’ value." This needs to be implemented.
Attachments
Test case (although should really be converted to a ref test) (907 bytes, text/html)
2011-11-29 05:20 PST, Aharon (Vladimir) Lanin 		no flags
Details
Work in progress (12.57 KB, patch)
2011-11-29 14:53 PST, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (20.98 KB, patch)
2011-11-29 17:37 PST, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (25.41 KB, patch)
2011-12-19 16:11 PST, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (25.41 KB, patch)
2011-12-19 16:43 PST, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (18.10 KB, patch)
2011-12-20 14:13 PST, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (18.15 KB, patch)
2012-01-24 16:51 PST, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (18.01 KB, patch)
2012-01-24 17:29 PST, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Aharon (Vladimir) Lanin
Comment 1 2011-11-29 05:20:39 PST
Created attachment 116949 [details] Test case (although should really be converted to a ref test)
Ryosuke Niwa
Comment 2 2011-11-29 10:38:55 PST
Do you men that both isolate and plaintext should work for inline element?
Levi Weintraub
Comment 3 2011-11-29 14:53:17 PST
Created attachment 117049 [details] Work in progress This currently crashes due, at least in part, to bug 69267.
Levi Weintraub
Comment 4 2011-11-29 17:37:22 PST
Created attachment 117083 [details] Patch This is still dependent upon 69267, and fails the included relayout test case. It's still a step in the right direction :)
Levi Weintraub
Comment 5 2011-11-29 17:38:40 PST
Comment on attachment 117083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117083&action=review > Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:74 > + #writer.write_image_diff_files(image_diff) This was done to get Ref Tests to not crash on the mac port. It won't be in the final patch :)
Aharon (Vladimir) Lanin
Comment 6 2011-11-30 00:47:50 PST
(In reply to comment #2) > Do you men that both isolate and plaintext should work for inline element? Yes. I personally don't see much point in using plaintext on an inline element, but the now-specified behavior makes sense if someone does use it that way.
Ryosuke Niwa
Comment 7 2011-11-30 14:41:26 PST
Comment on attachment 117083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117083&action=review > Source/WebCore/rendering/InlineIterator.h:134 > + if (unicodeBidi == Isolate || unicodeBidi == Plaintext) { Can we add isIsolate() to where EUnicodeBidi.h is defined? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:957 > // It does not matter which order we resolve the runs as long as we resolve them all. Doesn't your approach make the order to matter here? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:968 > + determineDirectionality(direction, InlineIterator(isolatedInline, (firstIsolatedRunOnLine) ? topResolver.firstIsolatedObjectOnLine() : bidiFirstSkippingEmptyInlines(isolatedInline), 0)); Don't need parentheses around firstIsolatedRunOnLine. > LayoutTests/fast/text/international/inline-plaintext-is-isolated-expected.html:1 > +<!DOCTYPE HTML> Remove BOM? > LayoutTests/fast/text/international/inline-plaintext-relayout-with-leading-neutrals-expected.html:1 > +<!DOCTYPE HTML> Ditto. > LayoutTests/fast/text/international/inline-plaintext-relayout-with-leading-neutrals.html:1 > +<!DOCTYPE HTML> Ditto. > LayoutTests/fast/text/international/inline-plaintext-with-generated-content-expected.html:1 > +<!DOCTYPE HTML> Ditto. > LayoutTests/fast/text/international/inline-plaintext-with-generated-content.html:1 > +<!DOCTYPE HTML> Ditto.
Levi Weintraub
Comment 8 2011-12-19 13:10:29 PST
Comment on attachment 117083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117083&action=review >> Source/WebCore/rendering/InlineIterator.h:134 >> + if (unicodeBidi == Isolate || unicodeBidi == Plaintext) { > > Can we add isIsolate() to where EUnicodeBidi.h is defined? For sure. This cleans things up. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:957 >> // It does not matter which order we resolve the runs as long as we resolve them all. > > Doesn't your approach make the order to matter here? No. Each isolated run is still independent. >> LayoutTests/fast/text/international/inline-plaintext-is-isolated-expected.html:1 >> +<!DOCTYPE HTML> > > Remove BOM? Good catch, thanks.
Levi Weintraub
Comment 9 2011-12-19 16:11:58 PST
Created attachment 119937 [details] Patch
Early Warning System Bot
Comment 10 2011-12-19 16:29:31 PST
Comment on attachment 119937 [details] Patch Attachment 119937 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10915221
Levi Weintraub
Comment 11 2011-12-19 16:43:05 PST
Created attachment 119949 [details] Patch
Ryosuke Niwa
Comment 12 2011-12-19 19:06:43 PST
Comment on attachment 119949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119949&action=review > Source/WebCore/ChangeLog:22 > + (WebCore::BidiResolver::BidiResolver): Added state for tracking the first object on a line > + after a hard line break in a bidi-isolated RenderInline. This is needed to keep track of where > + to start creating TextRuns in constructBidiRuns. It'll be helpful if you had an example for which this state is needed in the change log. > Source/WebCore/ChangeLog:45 > +2011-12-19 Levi Weintraub <leviw@chromium.org> > + > + unicode-bidi:plaintext is supposed to be effective on display:inline elements too > + https://bugs.webkit.org/show_bug.cgi?id=73310 > + > + Reviewed by NOBODY (OOPS!). change log dup. > Source/WebCore/rendering/InlineIterator.h:466 > + addPlaceholderRunForIsolatedInline(resolver, iter.object(), iter.offset()); > + resolver.setBeginningOfIsolatedRunOnLine(iter); Starting http://trac.webkit.org/changeset/102875, fake runs stores the offset in the object instead of just the object. Could you explain why we need to store a separate iterator in addition to what we already have in the changelog? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:977 > + else > + isolatedInline = toRenderInline(containingIsolate(isolatedRun->object(), currentRoot)); We should probably assert that unicodeBidi is Isolate in this case.
Levi Weintraub
Comment 13 2011-12-20 14:13:52 PST
Created attachment 120082 [details] Patch
Levi Weintraub
Comment 14 2012-01-11 10:11:02 PST
Ping reviewers.
Levi Weintraub
Comment 15 2012-01-18 11:17:16 PST
Another ping for a review.
Levi Weintraub
Comment 16 2012-01-24 16:51:10 PST
Created attachment 123843 [details] Patch
Levi Weintraub
Comment 17 2012-01-24 16:52:55 PST
(In reply to comment #16) > Created an attachment (id=123843) [details] > Patch Updating the patch to trunk. Eric, Ryosuke, anyone have some cycles to spare for a review?
Eric Seidel (no email)
Comment 18 2012-01-24 17:06:04 PST
Comment on attachment 123843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123843&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1672 > - // FIXME: Why does "unicode-bidi: plaintext" bidiFirstIncludingEmptyInlines when all other line layout code uses bidiFirstSkippingEmptyInlines? Why?
Eric Seidel (no email)
Comment 19 2012-01-24 17:06:55 PST
Otherwise the patch looks OK. Maybe levi answered my question before and I forgot?
Levi Weintraub
Comment 20 2012-01-24 17:08:49 PST
Comment on attachment 123843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123843&action=review >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1672 >> - // FIXME: Why does "unicode-bidi: plaintext" bidiFirstIncludingEmptyInlines when all other line layout code uses bidiFirstSkippingEmptyInlines? > > Why? D'oh!! When I updated, this was the one conflict. I removed the FIXME because in the previous version of the patch, I'd made this use bidiFirstSkippingEmptyInlines as the comment suggests it should. Let me fix that.
Levi Weintraub
Comment 21 2012-01-24 17:29:01 PST
Created attachment 123856 [details] Patch
Levi Weintraub
Comment 22 2012-01-24 17:30:24 PST
(In reply to comment #19) > Otherwise the patch looks OK. Maybe levi answered my question before and I forgot? Thanks for the good catch. The new patch should fix things.
Eric Seidel (no email)
Comment 23 2012-01-24 17:34:38 PST
Comment on attachment 123856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123856&action=review > Source/WebCore/rendering/InlineIterator.h:257 > +static inline RenderObject* bidiFirstSkippingEmptyInlines(RenderObject* root, InlineBidiResolver* resolver = 0) So why is this resolver=0 support needed?
Levi Weintraub
Comment 24 2012-01-24 17:37:34 PST
Comment on attachment 123856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123856&action=review >> Source/WebCore/rendering/InlineIterator.h:257 >> +static inline RenderObject* bidiFirstSkippingEmptyInlines(RenderObject* root, InlineBidiResolver* resolver = 0) > > So why is this resolver=0 support needed? Specifically for the callsite in determineStartPosition that used to be bidiFirstIncludingEmptyInlines. It doesn't have a resolver that should be modified.
Eric Seidel (no email)
Comment 25 2012-01-24 18:10:23 PST
Comment on attachment 123856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123856&action=review >>> Source/WebCore/rendering/InlineIterator.h:257 >>> +static inline RenderObject* bidiFirstSkippingEmptyInlines(RenderObject* root, InlineBidiResolver* resolver = 0) >> >> So why is this resolver=0 support needed? > > Specifically for the callsite in determineStartPosition that used to be bidiFirstIncludingEmptyInlines. It doesn't have a resolver that should be modified. So we have this strange scenerio where we need to determine the start position in order to determine the direction, and then we need the direction in order to configure the resolver as part of determining the start position? Am I understanding that correctly? Is there a better way we should be refactoring this (not necessarily in this patch)?
Levi Weintraub
Comment 26 2012-01-24 19:44:21 PST
Comment on attachment 123856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123856&action=review >>>> Source/WebCore/rendering/InlineIterator.h:257 >>>> +static inline RenderObject* bidiFirstSkippingEmptyInlines(RenderObject* root, InlineBidiResolver* resolver = 0) >>> >>> So why is this resolver=0 support needed? >> >> Specifically for the callsite in determineStartPosition that used to be bidiFirstIncludingEmptyInlines. It doesn't have a resolver that should be modified. > > So we have this strange scenerio where we need to determine the start position in order to determine the direction, and then we need the direction in order to configure the resolver as part of determining the start position? Am I understanding that correctly? Is there a better way we should be refactoring this (not necessarily in this patch)? Effectively yes. We want to position an Inline Iterator at the first position to run the generic UBA algorithm to determine the directionality of the paragraph (first strong character). When in plaintext, we use this to seed the resolver. The misnomer for me here is "determineStartPosition," as it clearly is doing a lot more than determining the start position for line layout; it's also setting a lot of state. Perhaps this could be cleared up with a bidiFirst that doesn't take a resolver, and that bidiFirstSkippingEmptyInlines uses. Then we could keep the assert and dump the if.
Levi Weintraub
Comment 27 2012-01-25 11:46:49 PST
(In reply to comment #26) > Perhaps this could be cleared up with a bidiFirst that doesn't take a resolver, and that bidiFirstSkippingEmptyInlines uses. Then we could keep the assert and dump the if. How does this sound for a refactoring? Want me to do that in this patch?
Levi Weintraub
Comment 28 2012-01-30 16:12:39 PST
(In reply to comment #27) > (In reply to comment #26) > > Perhaps this could be cleared up with a bidiFirst that doesn't take a resolver, and that bidiFirstSkippingEmptyInlines uses. Then we could keep the assert and dump the if. > > How does this sound for a refactoring? Want me to do that in this patch? This actually isn't really possible without duplicating some code. I could create a simple inline function that better describes the unique use case here and simply maps into bidiFirstSkippingEmptyInlines with no resolver.
Eric Seidel (no email)
Comment 29 2012-02-07 14:30:08 PST
Comment on attachment 123856 [details] Patch OK.
WebKit Review Bot
Comment 30 2012-02-07 15:46:57 PST
Comment on attachment 123856 [details] Patch Clearing flags on attachment: 123856 Committed r107000: <http://trac.webkit.org/changeset/107000>
WebKit Review Bot
Comment 31 2012-02-07 15:47:04 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.