Bug 121069 - If an element has unicode-bidi: plaintext and no strong directional characters. By default it should be LTR
Summary: If an element has unicode-bidi: plaintext and no strong directional character...
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: BlinkMergeCandidate
Depends on:
Blocks:
 
Reported: 2013-09-09 19:19 PDT by Ryosuke Niwa
Modified: 2022-08-21 13:31 PDT (History)
15 users (show)

See Also:


Attachments
Patch (14.23 KB, patch)
2013-09-10 12:25 PDT, Myles C. Maxfield
sam: review-
Details | Formatted Diff | Diff
Test results without source changed (but with new test applied) (418.76 KB, application/x-gzip)
2013-09-10 12:30 PDT, Myles C. Maxfield
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Myles C. Maxfield 2013-09-10 12:25:09 PDT
Created attachment 211221 [details]
Patch
Comment 2 Myles C. Maxfield 2013-09-10 12:30:32 PDT
Created attachment 211222 [details]
Test results without source changed (but with new test applied)
Comment 3 Ryosuke Niwa 2013-09-10 13:14:51 PDT
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.
Comment 4 Sam Weinig 2013-09-10 14:45:33 PDT
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.
Comment 5 Ahmad Saleem 2022-08-21 01:24:14 PDT
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.

rniwa@webkit.org - Can you confirm if this is needed anymore since it seems to be fixed in STP 151. Thanks!
Comment 6 Ryosuke Niwa 2022-08-21 12:33:13 PDT
I don't think it's my change but this is working now.
Comment 7 zalan 2022-08-21 13:31:18 PDT
IFC (modern line layout) made this path redundant.
Comment 8 zalan 2022-08-21 13:31:33 PDT
(In reply to zalan from comment #7)
> IFC (modern line layout) made this path redundant.
patch even.