73310 – unicode-bidi:plaintext is supposed to be effective on display:inline elements too

RESOLVED FIXED 73310

unicode-bidi:plaintext is supposed to be effective on display:inline elements too

https://bugs.webkit.org/show_bug.cgi?id=73310

Summary unicode-bidi:plaintext is supposed to be effective on display:inline elements...

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	Details Formatted Diff Diff
Patch (20.98 KB, patch) 2011-11-29 17:37 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (25.41 KB, patch) 2011-12-19 16:11 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (25.41 KB, patch) 2011-12-19 16:43 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (18.10 KB, patch) 2011-12-20 14:13 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (18.15 KB, patch) 2012-01-24 16:51 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (18.01 KB, patch) 2012-01-24 17:29 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
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.

Status RESOLVED

resolved as

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component CSS

Assignee

Levi Weintraub

Reported

2011-11-29 04:53 PST

Modified

2012-02-08 02:09 PST History

CC List

8 users Show

URL

Keywords

Depends on

69267 78092

Blocks

50910

Dependencies

tree graph