Bug 71194

(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?

(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).

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.

FYI, the corresponding bug was recently fixed in Blink. See https://code.google.com/p/chromium/issues/detail?id=280709.

Created attachment 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/

Adding Andreas and Zoltan to CC, as they seem to be quite active in this RenderBlockLineLayout.cpp file.

Comment on attachment 224487 [details] Patch proposal 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

Created attachment 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 on attachment 224487 [details] Patch proposal 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

Created attachment 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 on attachment 224487 [details] Patch proposal 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

Created attachment 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

Created attachment 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)

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!

Back from holidays. Any reviewer willing to take a look to this? PS: Adding Hyatt to CC too

Comment on attachment 224501 [details] Patch proposal + updated expectations for Mac-mountainlion 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 on attachment 224501 [details] Patch proposal + updated expectations for Mac-mountainlion 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

Created attachment 225150 [details] Patch proposal New patch addressing the issues mentioned during the review

(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

Ping reviewers?

Comment on attachment 225150 [details] Patch proposal r=me

Comment on attachment 225150 [details] Patch proposal Clearing flags on attachment: 225150 Committed r164867: <http://trac.webkit.org/changeset/164867>

All reviewed patches have been landed. Closing bug.

fast/text/international/unicode-bidi-plaintext-in-textarea.html is failing on Mavericks now.

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.

Did this fix also fix bug 78594?

(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