Bug 71194 - paragraphs with different directionality in textarea with unicode-bidi: plaintext are aligned the same
: paragraphs with different directionality in textarea with unicode-bidi: plain...
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 50910 129313
  Show dependency treegraph
 
Reported: 2011-10-30 14:24 PST by
Modified: 2014-03-06 05:49 PST (History)


Attachments
Patch proposal (18.68 KB, patch)
2014-02-18 02:37 PST, Mario Sanchez Prada
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (492.75 KB, application/zip)
2014-02-18 03:45 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (519.04 KB, application/zip)
2014-02-18 04:04 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (518.28 KB, application/zip)
2014-02-18 05:02 PST, Build Bot
no flags Details
Patch proposal + updated expectations for Mac-mountainlion (19.32 KB, patch)
2014-02-18 06:10 PST, Mario Sanchez Prada
no flags Review Patch | Details | Formatted Diff | Diff
Patch proposal (19.46 KB, patch)
2014-02-25 08:41 PST, Mario Sanchez Prada
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-10-30 14:24:25 PST
In a <textarea> with style="unicode-bidi: plaintext" the direction of each paragraph is supposed to be detected automatically. Currently all the paragraphs are aligned to the same side, even if their directionality is identified is different from that of the other paragraphs. It would make more sense to align RTL paragraphs to the right and LTR paragraphs to the left. This is how it is rendered in some Linux plain text editors that support RTL text and in Mozilla - currently "unicode-bidi: plaintext" is implemented in Firefox 10, which is the nightly test release.
------- Comment #1 From 2011-12-05 13:37:45 PST -------
(In reply to comment #0)
> In a <textarea> with style="unicode-bidi: plaintext" the direction of each paragraph is supposed to be detected automatically. Currently all the paragraphs are aligned to the same side, even if their directionality is identified is different from that of the other paragraphs. It would make more sense to align RTL paragraphs to the right and LTR paragraphs to the left. This is how it is rendered in some Linux plain text editors that support RTL text and in Mozilla - currently "unicode-bidi: plaintext" is implemented in Firefox 10, which is the nightly test release.

Should this happen on any element that doesn't specify text-align or direction? Are there relevant specs that specifiy this behavior?
------- Comment #2 From 2011-12-05 23:59:06 PST -------
(In reply to comment #1)
> Are there relevant specs that specifiy this behavior?

That's the way it was imagined when plaintext was first suggested, and that meeting's notes have the following formulation:

If the element's text-align or text-align-last is start or end, for every line box contained completely within the element, the alignment is determined by the paragraph level of the containing UBA paragraph that got its paragraph level from this element.

However, this is not a spec and has no actual standing. No mention of this behavior was ever made in the plaintext spec in CSS Writing Modes Level 3. That spec's editor, who also made the formulation above, is unsure why it was dropped from the spec. There has not yet been any guidance from the CSS WG on whether the spec is likely to be changed in this respect.

> Should this happen on any element that doesn't specify text-align or direction?

It should happen when the element's text-align (or text-align-last) is "start" or (in reverse) if it's "end". Please note that "start" is the default value of text-align for the root element, but from there on it is determined by inheritance when not explicitly specified. The start and end values normally work on the basis of the element's direction property, but the very nature of the suggestion is that in a unicode-bidi:plaintext element, the start and end values should work off the paragraph direction instead (and thus ignore the element's direction).
------- Comment #3 From 2012-02-12 04:06:58 PST -------
1. Please note that the paragraph direction sets alignment not only for "text-align:start" and "text-align:end", but also for "text-align:start end", as well as for "text-align-last:start" and "text-align-last:end".

2. I think that while the direction of a non-empty all-neutral paragraph should remain LTR as it is currently implemented, I think that there is room for "fudging" a little and making the direction of a completely empty (not just all-neutral) paragraph match the direction of the preceding paragraph, and if there is none, then the element's computed direction. The purpose of this would be just to set the alignment such that when typing a sequence of RTL paragraphs in a unicode-bidi:plaintext textarea, one will not initially start off with the caret on the left for each new paragraph. Other than alignment in a textarea, the direction of an empty paragraph is (I think) a moot point.
------- Comment #4 From 2013-10-02 03:13:32 PST -------
FYI, the corresponding bug was recently fixed in Blink. See https://code.google.com/p/chromium/issues/detail?id=280709.
------- Comment #5 From 2014-02-18 02:37:48 PST -------
Created an attachment (id=224487) [details]
Patch proposal

(In reply to comment #4)
> FYI, the corresponding bug was recently fixed in Blink.
> See https://code.google.com/p/chromium/issues/detail?id=280709.

Indeed. And I came across it when trying to backport a patch of mine [1] that depends on this change Igor committed in Blink a few months ago already.

Now attaching a patch backporting that commit from Blink, with just a few changes as needed, as the patch did not apply because some small differences, and also because unicode-bidi-plaintext.html is using a reftest now. Still, this will need rebasing in other platforms than GTK when landing.

Please review. Thanks!

[1] https://codereview.chromium.org/160583002/
------- Comment #6 From 2014-02-18 02:42:37 PST -------
Adding Andreas and Zoltan to CC, as they seem to be quite active in this RenderBlockLineLayout.cpp file.
------- Comment #7 From 2014-02-18 03:45:31 PST -------
(From update of attachment 224487 [details])
Attachment 224487 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6192756376469504

New failing tests:
fast/text/international/unicode-bidi-plaintext-in-textarea.html
------- Comment #8 From 2014-02-18 03:45:33 PST -------
Created an attachment (id=224489) [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
------- Comment #9 From 2014-02-18 04:04:23 PST -------
(From update of attachment 224487 [details])
Attachment 224487 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5854200881741824

New failing tests:
fast/text/international/unicode-bidi-plaintext-in-textarea.html
------- Comment #10 From 2014-02-18 04:04:28 PST -------
Created an attachment (id=224491) [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
------- Comment #11 From 2014-02-18 05:02:32 PST -------
(From update of attachment 224487 [details])
Attachment 224487 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6704411132821504

New failing tests:
fast/text/international/unicode-bidi-plaintext-in-textarea.html
------- Comment #12 From 2014-02-18 05:02:37 PST -------
Created an attachment (id=224498) [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
------- Comment #13 From 2014-02-18 06:10:24 PST -------
Created an attachment (id=224501) [details]
Patch proposal + updated expectations for Mac-mountainlion

Uploading a new patch with updating expectations for mountain lion, based in the output from the EWS (still will probably need rebaselining for other platforms)
------- Comment #14 From 2014-02-18 08:18:56 PST -------
I'm leaving the office now and will be off until Monday, so I won't be able to commit this before the next week if it gets approved. Should that be the case, feel free to cq+ (if you are willing to monitor the bots) if you want it in asap, or I'll do it next week.

Thanks!
------- Comment #15 From 2014-02-24 01:29:42 PST -------
Back from holidays. Any reviewer willing to take a look to this?

PS: Adding Hyatt to CC too
------- Comment #16 From 2014-02-25 08:02:01 PST -------
(From update of attachment 224501 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=224501&action=review

r- just for the FIXME comment that I'd like to see.

Note this code could in theory work without needing a root line box (for the one place that did not get patched properly). If you knew the bidi level, you can compute everything else, since the style is the RenderBlockFlow's style.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:580
> +    TextDirection direction;
> +    if (rootInlineBox && rootInlineBox->renderer().style().unicodeBidi() == Plaintext)
> +        direction = rootInlineBox->direction();
> +    else
> +        direction = style().direction();

The renderer for a root line box is the RenderBlockFlow, so I think this would read better as:

if (rootInlineBox && style().unicodeBidi() == Plaintext)

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2175
> -    updateLogicalWidthForAlignment(textAlign, 0, logicalLeft, totalLogicalWidth, availableLogicalWidth, 0);
> +    updateLogicalWidthForAlignment(textAlign, 0, 0, logicalLeft, totalLogicalWidth, availableLogicalWidth, 0);

This needs a FIXME, since it's obviously wrong. You pass no root line box here. All you need is the hypothetical bidi level that would be used to construct a line here. I would think that could be determined.
------- Comment #17 From 2014-02-25 08:29:51 PST -------
(From update of attachment 224501 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=224501&action=review

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:580
>> +        direction = style().direction();
> 
> The renderer for a root line box is the RenderBlockFlow, so I think this would read better as:
> 
> if (rootInlineBox && style().unicodeBidi() == Plaintext)

Ok

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2175
>> +    updateLogicalWidthForAlignment(textAlign, 0, 0, logicalLeft, totalLogicalWidth, availableLogicalWidth, 0);
> 
> This needs a FIXME, since it's obviously wrong. You pass no root line box here. All you need is the hypothetical bidi level that would be used to construct a line here. I would think that could be determined.

Ok. Since this patch is a backport from Blink, I'll report a bug and add that FIXME here, so we can track this down
------- Comment #18 From 2014-02-25 08:41:15 PST -------
Created an attachment (id=225150) [details]
Patch proposal

New patch addressing the issues mentioned during the review
------- Comment #19 From 2014-02-25 12:33:38 PST -------
(In reply to comment #17)
> [...]
> > This needs a FIXME, since it's obviously wrong. You pass no root line box here. All you need is 
> > the hypothetical bidi level that would be used to construct a line here. I would think that could
> > be determined.
> 
> Ok. Since this patch is a backport from Blink, I'll report a bug and add that FIXME here, so we
> can track this down

Besides attaching a new patch, I also reported a bug to track down that incompleteness, as promised: see bug 129311
------- Comment #20 From 2014-02-26 07:44:32 PST -------
Ping reviewers?
------- Comment #21 From 2014-02-28 09:31:48 PST -------
(From update of attachment 225150 [details])
r=me
------- Comment #22 From 2014-02-28 10:07:19 PST -------
(From update of attachment 225150 [details])
Clearing flags on attachment: 225150

Committed r164867: <http://trac.webkit.org/changeset/164867>
------- Comment #23 From 2014-02-28 10:07:26 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #24 From 2014-02-28 10:47:38 PST -------
fast/text/international/unicode-bidi-plaintext-in-textarea.html is failing on Mavericks now.
------- Comment #25 From 2014-02-28 10:53:21 PST -------
Updated the results in <http://trac.webkit.org/r164872>.

I don't know if the new results are correct, but the changes are similar to the changes landed for 10.8 in the patch, just slightly different metrics.
------- Comment #26 From 2014-03-02 01:12:31 PST -------
Did this fix also fix bug 78594?
------- Comment #27 From 2014-03-06 05:49:19 PST -------
(In reply to comment #26)
> Did this fix also fix bug 78594?

I have run MiniBrowser just now after a full build against trunk and the test case in [1] is still not properly rendered.

So no, this fix hasn't fixed that bug yet :(

[1] https://bugs.webkit.org/attachment.cgi?id=126948