Bug 50949 – Add support for unicode-bidi:plaintext CSS property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Xiaomei Ji]

Following is from CSS3 spec:

plaintext
For the purposes of the Unicode bidirectional algorithm, the base directionality of each "paragraph" for which the element is the containing block element is determined not by the element's computed ‘direction’ as usual, but by following rules P1, P2, and P3 of the Unicode bidirectional algorithm. However, if no direction-determining character is found in step P2, then the value of the ‘direction’ property is used instead. Note this value has no effect on inline elements.

[Levi Weintraub]

Created attachment 88270 [details]
Patch

[WebKit Review Bot]

Attachment 88270 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/rendering/style/RenderStyle.h:259:  _unicodeBidi is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Levi Weintraub]

Comment on attachment 88270 [details]
Patch

Canceling the review as there were two more changes to make:
1 - check for line breaks and bail in determineParagraphDirection, since in the first layout we don't have an end iterator
2 - properly set unicode-bidi to plaintext for pre and textarea.

New patch coming shortly.

[Levi Weintraub]

Created attachment 88417 [details]
Patch

[WebKit Review Bot]

Attachment 88417 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/rendering/style/RenderStyle.h:259:  _unicodeBidi is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Levi Weintraub]

I'd love a review if anyone gets a chance :)

[Eric Seidel (no email)]

Comment on attachment 88417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88417&action=review

I'm concerned this is not the only place you want this code.  Otherwise seems fine.

> Source/WebCore/html/HTMLElement.cpp:170
> +        int bidiAttribute;
> +        if (hasLocalName(bdoTag))
> +            bidiAttribute = CSSValueBidiOverride;
> +        else if (hasLocalName(preTag) || hasLocalName(textareaTag))
> +            bidiAttribute = CSSValueWebkitPlaintext;
> +        else
> +            bidiAttribute = CSSValueEmbed;
> +        addCSSProperty(attr, CSSPropertyUnicodeBidi, bidiAttribute);

This is a super-weird place for this code!  I guess since I we have eto leave his here we should abstract this into a unicodeBidFromDirAuto() helper.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:87
> +static void determineParagraphDirection(Direction& dir, InlineIterator iter)
> +{

So this is basically "first strong direction in paragarph"?  I'm not sure where this logic goes.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1266
> +        if (style()->unicodeBidi() == Plaintext)
> +            determineParagraphDirection(direction, InlineIterator(this, bidiFirst(this, 0), pos));

I think we're going to want to start passing unicodeBidi() to BidiContexts :(

Are we sure this is the only place this needs to be done?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1270
>          resolver.setLastStrongDir(direction);
>          resolver.setLastDir(direction);
>          resolver.setEorDir(direction);
> -        resolver.setContext(BidiContext::create(ltr ? 0 : 1, direction, style()->unicodeBidi() == Override, FromStyleOrDOM));
> +        resolver.setContext(BidiContext::create(direction == LeftToRight ? 0 : 1, direction, style()->unicodeBidi() == Override, FromStyleOrDOM));

I think I have an outstanding conflicting patch here.

[Levi Weintraub]

Comment on attachment 88417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88417&action=review

>> Source/WebCore/html/HTMLElement.cpp:170
>> +        addCSSProperty(attr, CSSPropertyUnicodeBidi, bidiAttribute);
> 
> This is a super-weird place for this code!  I guess since I we have eto leave his here we should abstract this into a unicodeBidFromDirAuto() helper.

That makes sense.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:87
>> +{
> 
> So this is basically "first strong direction in paragarph"?  I'm not sure where this logic goes.

That's correct. First the first character with a strong directionality is what determines the paragraph's base writing direction.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1266
>> +            determineParagraphDirection(direction, InlineIterator(this, bidiFirst(this, 0), pos));
> 
> I think we're going to want to start passing unicodeBidi() to BidiContexts :(
> 
> Are we sure this is the only place this needs to be done?

These two cases cover it: when we initially set up the BidiContext for the block, and again after each paragraph separator.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1270
>> +        resolver.setContext(BidiContext::create(direction == LeftToRight ? 0 : 1, direction, style()->unicodeBidi() == Override, FromStyleOrDOM));
> 
> I think I have an outstanding conflicting patch here.

Link? If it's going to be a blocking issue list it as such so I know when to update this patch.

[Levi Weintraub]

Created attachment 91362 [details]
Patch

[WebKit Review Bot]

Attachment 91362 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/style/RenderStyle.h:259:  _unicodeBidi is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Levi Weintraub]

@Eric: Following up wondering about the conflicting patch you had on this. When does it land? Could you add it as a blocker so I can track it?

I think I cleared up your other issue and appreciate your feedback :)

[Levi Weintraub]

Created attachment 98088 [details]
Patch

[WebKit Review Bot]

Attachment 98088 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/style/RenderStyle.h:259:  _unicodeBidi is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 98088 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98088&action=review

I think I need to look at this again later.

> Source/WebCore/html/HTMLElement.cpp:129
> +static int unicodeBidiAttributeForDirAuto(HTMLElement* element)

Should this be marked inline?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:115
> +static void determineParagraphDirection(TextDirection& dir, InlineIterator iter)

Should InlineIterator be passed as a reference?  do you actually want it to be copied here?  Maybe that would be more clear with a const & which you explicitly copy?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:967
> +        bool isNewParagraph = lineInfo.previousLineBrokeCleanly();

is this true?  that it's a "new paragraph"?  You might want to say isNewUBAParagrah?  I'm not sure.  Do we need a new accessor on lineInfo which talks about UBA things instead of previous line?  They may be backed by the same bool, but I wonder if we shouldn't have a UBA-specific accessor on lineInfo, since it's not clear from previousLineBrokeCleanly what it applies to...

> Source/WebCore/rendering/style/RenderStyle.h:261
> -        // 50 bits
> +        // 51 bits

Did you validate this count?  Sometimes these comments are out of date.

[WebKit Review Bot]

Comment on attachment 98088 [details]
Patch

Attachment 98088 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8926094

New failing tests:
fast/text/international/unicode-bidi-plaintext.html

[WebKit Review Bot]

Created attachment 98093 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Levi Weintraub]

Created attachment 98211 [details]
Patch

[Levi Weintraub]

Comment on attachment 98211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98211&action=review

> Source/WebCore/rendering/style/RenderStyle.h:261
> +        // 53 bits

It really is 53 bits now... The comment was off by 2!

[WebKit Review Bot]

Attachment 98211 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/style/RenderStyle.h:259:  _unicodeBidi is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 98211 [details]
Patch

Attachment 98211 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8925650

New failing tests:
fast/text/international/unicode-bidi-plaintext.html

[WebKit Review Bot]

Created attachment 98215 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Seidel (no email)]

Comment on attachment 98211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98211&action=review

r- for the cr-linux failure.  Happy to look again after your replies.

> Source/WebCore/html/HTMLElement.cpp:129
> +static inline int unicodeBidiAttributeForDirAuto(HTMLElement* element)

So sad that these are ints, and not an enum.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:967
> +        bool isNewUBAParagraph = lineInfo.previousLineBrokeCleanly();

I wonder if this shouldn't be an accessor on lineInfo.  So that at some point later we might distinguish between nextLineIsNewUBAParagraph and previousLineBrokeCleanly (since I think they're used for separate things a times).

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:991
> +            if (isNewUBAParagraph && style()->unicodeBidi() == Plaintext && !resolver.context()->parent()) {
> +                TextDirection direction = style()->direction();
> +                determineParagraphDirection(direction, resolver.position());
> +                resolver.setStatus(BidiStatus(direction, style()->unicodeBidi() == Override));
> +            }

Why is this needed here?  I would think you would only need this in determinedStart... below.

[Levi Weintraub]

Comment on attachment 98211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98211&action=review

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:967
>> +        bool isNewUBAParagraph = lineInfo.previousLineBrokeCleanly();
> 
> I wonder if this shouldn't be an accessor on lineInfo.  So that at some point later we might distinguish between nextLineIsNewUBAParagraph and previousLineBrokeCleanly (since I think they're used for separate things a times).

I'd agree this should be refactored when another use case crops up.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:991
>> +            }
> 
> Why is this needed here?  I would think you would only need this in determinedStart... below.

On all other modes, hitting a hard line break resets the UBA to the state at the beginning of the current block, but when in Unicode: plaintext mode, we recalculate the direction of this new paragraph and set the status to an embedding level of 0/1 depending on the first character with strong directionality.

[Levi Weintraub]

(In reply to comment #22)
> (From update of attachment 98211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98211&action=review
> 
> r- for the cr-linux failure.  Happy to look again after your replies.

I'm actually really confused by this cr-linux failure... It's failing the new test I uploaded, but I don't have expected results for linux!

[Levi Weintraub]

(In reply to comment #24)
> I'm actually really confused by this cr-linux failure... It's failing the new test I uploaded, but I don't have expected results for linux!

Can someone with a better understanding of the cr-linux ews bot's layout test functionality help me understand this?  Eric, Adam, Dmitry, I'm looking at you ;)

[Tony Chang]

You answered your own question, it failed because the new test you added doesn't have results for Chromium Linux.  When the test runs, it falls back to the mac result and it doesn't match, so it fails.

You can grab the result out of the zip file and add it to your patch and it should pass.  Alternately you could skip the test on Chromium Linux.

[Levi Weintraub]

(In reply to comment #26)
> You answered your own question, it failed because the new test you added doesn't have results for Chromium Linux.  When the test runs, it falls back to the mac result and it doesn't match, so it fails.
> 
> You can grab the result out of the zip file and add it to your patch and it should pass.  Alternately you could skip the test on Chromium Linux.

Oof! That seems like like a false-negative to go red for missing platform test expectations. While I love having the expected results generated, it's quite annoying if I'm getting r-'s for not doing my building on Linux Chrome :p

[Levi Weintraub]

Created attachment 98433 [details]
Patch

[Levi Weintraub]

(In reply to comment #28)
> Created an attachment (id=98433) [details]
> Patch

Including text cr-linux results...

[WebKit Review Bot]

Attachment 98433 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/style/RenderStyle.h:259:  _unicodeBidi is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 98433 [details]
Patch

Attachment 98433 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8935190

New failing tests:
fast/text/international/unicode-bidi-plaintext.html

[WebKit Review Bot]

Created attachment 98439 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Levi Weintraub]

Created attachment 98760 [details]
Patch

[Levi Weintraub]

Woo! Finally made cr-linux happy :)

[Eric Seidel (no email)]

Comment on attachment 98760 [details]
Patch

Seems OK.  You will need to watch the bots, as you may break others than just linux due to font differences.  I expect you'll break cr-win.

[Levi Weintraub]

Comment on attachment 98760 [details]
Patch

Thanks for the review! I'll keep an eye on the bots, I'm sure there'll be different platform expectations :(

[WebKit Review Bot]

Comment on attachment 98760 [details]
Patch

Clearing flags on attachment: 98760

Committed r89864: <http://trac.webkit.org/changeset/89864>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.