Bug 73310 - unicode-bidi:plaintext is supposed to be effective on display:inline elements too
Summary: unicode-bidi:plaintext is supposed to be effective on display:inline elements...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on: 69267 78092
Blocks: 50910
  Show dependency treegraph
 
Reported: 2011-11-29 04:53 PST by Aharon (Vladimir) Lanin
Modified: 2012-02-08 02:09 PST (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Aharon (Vladimir) Lanin 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.
Comment 1 Aharon (Vladimir) Lanin 2011-11-29 05:20:39 PST
Created attachment 116949 [details]
Test case (although should really be converted to a ref test)
Comment 2 Ryosuke Niwa 2011-11-29 10:38:55 PST
Do you men that both isolate and plaintext should work for inline element?
Comment 3 Levi Weintraub 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.
Comment 4 Levi Weintraub 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 :)
Comment 5 Levi Weintraub 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 :)
Comment 6 Aharon (Vladimir) Lanin 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.
Comment 7 Ryosuke Niwa 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.
Comment 8 Levi Weintraub 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.
Comment 9 Levi Weintraub 2011-12-19 16:11:58 PST
Created attachment 119937 [details]
Patch
Comment 10 Early Warning System Bot 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
Comment 11 Levi Weintraub 2011-12-19 16:43:05 PST
Created attachment 119949 [details]
Patch
Comment 12 Ryosuke Niwa 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.
Comment 13 Levi Weintraub 2011-12-20 14:13:52 PST
Created attachment 120082 [details]
Patch
Comment 14 Levi Weintraub 2012-01-11 10:11:02 PST
Ping reviewers.
Comment 15 Levi Weintraub 2012-01-18 11:17:16 PST
Another ping for a review.
Comment 16 Levi Weintraub 2012-01-24 16:51:10 PST
Created attachment 123843 [details]
Patch
Comment 17 Levi Weintraub 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?
Comment 18 Eric Seidel (no email) 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?
Comment 19 Eric Seidel (no email) 2012-01-24 17:06:55 PST
Otherwise the patch looks OK.  Maybe levi answered my question before and I forgot?
Comment 20 Levi Weintraub 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.
Comment 21 Levi Weintraub 2012-01-24 17:29:01 PST
Created attachment 123856 [details]
Patch
Comment 22 Levi Weintraub 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.
Comment 23 Eric Seidel (no email) 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?
Comment 24 Levi Weintraub 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.
Comment 25 Eric Seidel (no email) 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)?
Comment 26 Levi Weintraub 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.
Comment 27 Levi Weintraub 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?
Comment 28 Levi Weintraub 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.
Comment 29 Eric Seidel (no email) 2012-02-07 14:30:08 PST
Comment on attachment 123856 [details]
Patch

OK.
Comment 30 WebKit Review Bot 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>
Comment 31 WebKit Review Bot 2012-02-07 15:47:04 PST
All reviewed patches have been landed.  Closing bug.