Bug 220241

Summary: Web Inspector: RTL content inside elements is reversed and unreadable
Product: WebKit Reporter: Ebrahim Byagowi <ebrahim>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, gsnedders, inspector-bugzilla-changes, mmaxfield, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 14   
Hardware: All   
OS: All   
Attachments:
Description Flags
RTL text inside the element is reversed and is unreadable
none
Patch
none
Screenshot none

Description Ebrahim Byagowi 2021-01-04 07:13:29 PST
Created attachment 416940 [details]
RTL text inside the element is reversed and is unreadable

The fix in https://bugs.webkit.org/show_bug.cgi?id=207214 has caused Web Inspector to display RTL text completely reversed making RTL text content used inside elements completely unreadable. There is fortunately a solution to this that won't regress the fix there and to use "unicode-bidi: isolate" instead "unicode-bidi: isolate-override;" and I've applied the fix locally checking whether it won't regress the original issue and also fixes this disturbing issue.

Test page used in the attachment to show the issue:

  data:text/html;charset=utf8,سیب

"unicode-bidi: bidi-override" (and now the recently introduced isolate-override) is really something of past and was designed to make compatibility with text written before bidi and unicode era and is something should be and is avoided by modern web development.
Comment 1 Ebrahim Byagowi 2021-01-04 07:51:05 PST
Created attachment 416942 [details]
Patch
Comment 2 BJ Burg 2021-01-05 09:56:33 PST
(In reply to Ebrahim Byagowi from comment #0)
> Created attachment 416940 [details]
> RTL text inside the element is reversed and is unreadable
> 
> The fix in https://bugs.webkit.org/show_bug.cgi?id=207214 has caused Web
> Inspector to display RTL text completely reversed making RTL text content
> used inside elements completely unreadable. There is fortunately a solution
> to this that won't regress the fix there and to use "unicode-bidi: isolate"
> instead "unicode-bidi: isolate-override;" and I've applied the fix locally
> checking whether it won't regress the original issue and also fixes this
> disturbing issue.
> 
> Test page used in the attachment to show the issue:
> 
>   data:text/html;charset=utf8,سیب
> 
> "unicode-bidi: bidi-override" (and now the recently introduced
> isolate-override) is really something of past and was designed to make
> compatibility with text written before bidi and unicode era and is something
> should be and is avoided by modern web development.

Thanks for investigating this. I noticed that Web Inspector is in LTR layout mode in your screenshots. That's fine, but could you please ensure that the behavior is also correct when in RTL layout mode?
Comment 3 Radar WebKit Bug Importer 2021-01-05 09:56:50 PST
<rdar://problem/72817371>
Comment 4 Ebrahim Byagowi 2021-01-06 08:41:40 PST
> Thanks for investigating this. I noticed that Web Inspector is in LTR layout mode in your screenshots. That's fine, but could you please ensure that the behavior is also correct when in RTL layout mode?

Thank you for reaching to this :) Does Web Inspector even have a RTL mode? This fix won't have anything to do with it AFAICT still how can I run Web Inspector in RTL mode? I am using the run-minibrowser for testings and changing system language didn't have any apparent effect to it.
Comment 5 Myles C. Maxfield 2021-01-12 13:02:02 PST
If you set your system language to Arabic and restart Safari, there should be a big change. See the screenshot I attached.

You're right that RTL text looks broken when the inspector is in LTR mode. We just want to make sure that this patch doesn't regress behavior when the inspector is in RTL mode.
Comment 6 Myles C. Maxfield 2021-01-12 13:02:26 PST
Created attachment 417483 [details]
Screenshot
Comment 7 Sam Sneddon [:gsnedders] 2021-01-13 08:08:39 PST
We previously fixed a similar issue in bug 170298 by adding bdo=auto to the DOM tree (which in the UA stylesheet adds unicode-bidi: isolate).

Bug 207214, which introduced this regression, was outright wrong using isolate-override (it only makes sense in combination with an explicitly set direction, which we don't have for page content). In general, using the dir attribute (and potentially the bdo/bdi elements) is recommended given it's generally more obvious about what it does.

There's probably a strong argument that if the page has <span dir=rtl>foo</span> then we want to end up with <span class="html-text-node" dir=rtl>foo</span> in the Inspector, rather than using auto there. That said, with the patch attached to this bug, we're in a better place than before bug 207214, so I'm a non-reviewer r+ on this.
Comment 8 BJ Burg 2021-01-13 14:07:15 PST
Comment on attachment 416942 [details]
Patch

r=me, thanks for the help everyone.
Comment 9 EWS 2021-01-13 14:42:38 PST
Committed r271458: <https://trac.webkit.org/changeset/271458>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416942 [details].