Bug 121069 – If an element has unicode-bidi: plaintext and no strong directional characters. By default it should be LTR

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: Layout and Rendering

[Ryosuke Niwa]

Consider merging https://chromium.googlesource.com/chromium/blink/+/ad5ac746dbad9e71667dc873afbc0900d89c4a28

[Myles C. Maxfield]

Created attachment 211221 [details]
Patch

[Myles C. Maxfield]

Created attachment 211222 [details]
Test results without source changed (but with new test applied)

[Ryosuke Niwa]

Comment on attachment 211221 [details]
Patch

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

> Source/WebCore/css/html.css:1150
> +textarea[dir=auto] {
> +    unicode-bidi: -webkit-plaintext;
> +}

The change log entry should mention about this change.

> Source/WebCore/html/HTMLElement.cpp:835
> +    if (hasTagName(inputTag)) {

Use isHTMLInputElement instead.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:396
> +            && (current->isRenderInline() || current->isRenderBlock())) {

Why do we need to check for this condition?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:400
> +            if (current != root)
> +                current = 0;
> +            else
> +                current = next;

This is a very mysterious code.  It walks up the render tree to find the first inline or block render object with unicode-bidi: -webkit-isolate, and then if it's root, set current to the original value.  If not, then it resets (effectively setting it to root's firstChild).

According to https://code.google.com/p/chromium/issues/detail?id=277687, we should be looking for the first strongly LTR or RTL letter.  Given that, I don't see how this code can be correct.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:409
> +    if (!current)
> +        current = root->firstChild();
> +
> +    while (current) {

I don't like the fact current & next are re-purposed in the second loop. We should probably split either part into a separate function, and give much more descriptive variable names.

[Sam Weinig]

Comment on attachment 211221 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        * css/html.css:
> +        (textarea[dir=auto]):
> +        * html/HTMLElement.cpp:
> +        (WebCore::HTMLElement::directionality):
> +        * rendering/RenderBlockLineLayout.cpp:
> +        (WebCore::firstRenderObjectForDirectionalityDetermination):
> +        (WebCore::determinePlaintextDirectionality):
> +        (WebCore::constructBidiRunsForSegment):
> +        (WebCore::RenderBlock::layoutRunsAndFloatsInRange):
> +        (WebCore::RenderBlock::determineStartPosition):

Especially given that this is merge, I think you should really have per-change comments explaining the rationale.

[Ahmad Saleem]

I changed the test case from Chromium Patch to following JSFiddle:

Test - https://jsfiddle.net/jr0bv9h1/show

I think it is fixed via recent work around directionality and bid by (@rniwa) since Safari Technology Preview 151 is showing it similar to all other browsers (Chrome Canary 106 and Firefox Nightly 105), note position of !, which is of right side in STP 151 and others but on left side in Safari 15.6.1.

[e-mail] - Can you confirm if this is needed anymore since it seems to be fixed in STP 151. Thanks!

[Ryosuke Niwa]

I don't think it's my change but this is working now.

[zalan]

IFC (modern line layout) made this path redundant.

[zalan]

(In reply to zalan from comment #7)
> IFC (modern line layout) made this path redundant.
patch even.