Bug 220639 - Web Inspector: BiDi issue on JavaScript evaluation result element
Summary: Web Inspector: BiDi issue on JavaScript evaluation result element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: Safari 14
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-14 16:02 PST by Ebrahim Byagowi
Modified: 2021-01-21 17:24 PST (History)
6 users (show)

See Also:


Attachments
actual (17.41 KB, image/png)
2021-01-14 16:02 PST, Ebrahim Byagowi
no flags Details
PATCH (1.64 KB, patch)
2021-01-16 01:18 PST, Ebrahim Byagowi
no flags Details | Formatted Diff | Diff
PATCH (1.65 KB, patch)
2021-01-16 01:27 PST, Ebrahim Byagowi
bburg: review+
Details | Formatted Diff | Diff
PATCH (1.66 KB, patch)
2021-01-19 14:27 PST, Ebrahim Byagowi
no flags Details | Formatted Diff | Diff
PATCH (1.46 KB, patch)
2021-01-20 09:17 PST, Ebrahim Byagowi
no flags Details | Formatted Diff | Diff
PATCH (1.31 KB, patch)
2021-01-20 10:03 PST, Ebrahim Byagowi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ebrahim Byagowi 2021-01-14 16:02:13 PST
Created attachment 417665 [details]
actual

Steps to reproduce:
Open JavaScript console of WebInspector
Enter "سیب" (with its quotations) on evaluation line.

Actual:
"سیب" = $2

Expected:
"سیب" = $@
(I mean $2 by $@ just that numbers confuses bidi algorithm)

The solution is to apply unicode-bidi: isolate to the specific element.

I like to submit a patch for fixing this just that I don't know yet how to open a Web Inspector on Web Inspector :) on Chrome that can be achieved by entering Command+Option+I twice but in Safari that won't work apparently.
Comment 1 Sam Sneddon [:gsnedders] 2021-01-15 08:01:37 PST
> I like to submit a patch for fixing this just that I don't know yet how to open a Web Inspector on Web Inspector :) on Chrome that can be achieved by entering Command+Option+I twice but in Safari that won't work apparently.

On macOS, run:

defaults write NSGlobalDomain WebKitDebugDeveloperExtrasEnabled -bool YES

Then you'll get the "Inspect Element" option in the context menu.

> The solution is to apply unicode-bidi: isolate to the specific element.

I'd rather we added dir=auto as an attribute, rather than manually setting unicode-bidi: isolate, FWIW.

Regardless—thanks for filing bugs (and even more so patches!) for all these bidi issues!
Comment 2 Ebrahim Byagowi 2021-01-15 16:50:46 PST
Thank you very much! Okay now that I have a working Web Inspector development environment using your instruction I see there is one case that dir="auto"/<bdi>/unicode-bidi:isolate would be overkill and that is a string that itself ends with bidi natural character like,

"سیب."

If we use isolate it will turn it to

".سیب"

but there is a solution to this and is to use either unicode-bidi:embed or just to use dir="ltr" which implies unicode-bidi:embed anyway so I like to suggest something like

dir="ltr":

--- a/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js
+++ b/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js
@@ -532,6 +532,7 @@ WI.ConsoleMessageView = class ConsoleMessageView extends WI.Object
             parameters[i] = this._createRemoteObjectIfNeeded(parameters[i]);
 
         let builderElement = element.appendChild(document.createElement("span"));
+        builderElement.setAttribute("dir", "ltr");
         let shouldFormatWithStringSubstitution = parameters[0].type === "string" && this._message.type !== WI.ConsoleMessage.MessageType.Result;
 
         // Single object (e.g. console result or logging a non-string object).

Or, adding unicode-bidi:embed with CSS:

--- a/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css
+++ b/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css
@@ -309,6 +309,10 @@
     display: inline-block;
 }
 
+.console-message-content {
+    unicode-bidi: embed;
+}
+
 @media (prefers-color-scheme: dark) {
     .console-message .syntax-highlighted {
         background-color: unset;

--- a/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js
+++ b/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js
@@ -532,6 +532,7 @@ WI.ConsoleMessageView = class ConsoleMessageView extends WI.Object
             parameters[i] = this._createRemoteObjectIfNeeded(parameters[i]);
 
         let builderElement = element.appendChild(document.createElement("span"));
+        builderElement.classList.add("console-message-content");
         let shouldFormatWithStringSubstitution = parameters[0].type === "string" && this._message.type !== WI.ConsoleMessage.MessageType.Result;
 
         // Single object (e.g. console result or logging a non-string object).

Or do you prefer dir="auto", which is OK to me also. Thanks!
Comment 3 Ebrahim Byagowi 2021-01-16 01:18:53 PST
Created attachment 417762 [details]
PATCH

Here is a proposed fix that applies dir="auto" to the saved variable name so that bidi algorithm won't be confused when the actually saved string has RTL text.
Comment 4 Ebrahim Byagowi 2021-01-16 01:27:13 PST
Created attachment 417763 [details]
PATCH

Here is a proposed fix that applies dir="auto" to the saved variable name so that bidi algorithm won't be confused when the actually saved string has RTL text.
Comment 5 BJ Burg 2021-01-19 12:52:16 PST
Comment on attachment 417763 [details]
PATCH

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:400
> +        // So that bidi algorithm won't be confused when a saved string has RTL text

Nit: "so that the bidi"

Nit: WebKit style is to use a trailing period.
Comment 6 BJ Burg 2021-01-19 12:52:31 PST
Comment on attachment 417763 [details]
PATCH

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:400
> +        // So that bidi algorithm won't be confused when a saved string has RTL text

Nit: "so that the bidi"

Nit: WebKit style is to use a trailing period.
Comment 7 BJ Burg 2021-01-19 12:53:08 PST
(Apologies for the double comment, not sure what happened!)
Comment 8 Sam Sneddon [:gsnedders] 2021-01-19 13:15:00 PST
BJ, do we not want to apply this to .formatted-string?

> Or do you prefer dir="auto", which is OK to me also. Thanks!

We definitely don't want unicode-bidi: embed for anything, I'd think, given we don't want the larger context to influence it (or it to influence the larger context).

We might even want to split .formatted-string up so that the quotation marks don't have this applied to them.
Comment 9 Ebrahim Byagowi 2021-01-19 14:25:09 PST
> BJ, do we not want to apply this to .formatted-string?

It is OK to me just that in the following image, all the lines are from the same text "سیب." (means apple if you wonder)

https://i.imgur.com/sAqbjNx.png

0. "سیب."
1. The current situation, very bad, the bidi algorithm needs a help and $2 is mixed with the whole thing
2. If dir=auto be applied to .formatted-string
3. Current patch, what I see is expected that dir=auto is applied to $2's element, is achievable also if I could apply unicode-bidi:embed here which is considered not nice but I believe makes sense, it isn't that dangerous, web used to love it before isolate existence and its semantic is also OK here as it is just like applying dir=ltr here which itself is a safe thing, still.

However if you thinks 2 is OK I am also with it also, as it is definitely much better than 1 anyway.

Thanks!
Comment 10 Ebrahim Byagowi 2021-01-19 14:27:24 PST
Created attachment 417913 [details]
PATCH
Comment 11 Ebrahim Byagowi 2021-01-20 09:16:57 PST
Actually all we need here is to use unicode-bidi: isolate but not dir=auto which we makes the text to be represented incorrectly, so let me propose a patch based on that which I believe will be safest thing to do here.
Comment 12 Ebrahim Byagowi 2021-01-20 09:17:37 PST
Created attachment 417975 [details]
PATCH
Comment 13 Ebrahim Byagowi 2021-01-20 10:03:47 PST
Created attachment 417979 [details]
PATCH

It turned out the same issue can happen if regexp contains RTL script so updated the patch to consider that case also, the following image shows the issue.

https://i.imgur.com/hRAALOj.png

The text on the picture:
"سیب."
/سیب./

On the left, Safari, what currently happens, and on the right, Web Inspector of minibrowser, what I believe is expected behavior here.

Sorry for the unintended spam btw.
Comment 14 Sam Sneddon [:gsnedders] 2021-01-21 06:37:57 PST
Oh, no, I finally sat down and thought about this without getting interrupted and I'm sorry for being wrong before!

We definitely want the base direction to be ltr, and we want it to be isolated from the surrounding context, as otherwise things get very surprising in JS context. So we want unicode-bidi: isolate; direction: ltr; as the used values (i.e., dir=ltr), and direction: ltr is the initial value we indeed inherit so this current patch looks good to me.

So: non-reviewer r+.
Comment 15 fantasai 2021-01-21 14:40:21 PST
Isolation is definitely the correct thing to do here. I would say in general, if there's anywhere in your UI where you're presenting user-generated strings inline with other content, you want to isolate.

I'm a little unsure why you don't want dir=auto here, though. If you're dealing with RTL strings, wouldn't you want them handled as RTL?
Comment 16 BJ Burg 2021-01-21 14:40:35 PST
Comment on attachment 417979 [details]
PATCH

r=me
Comment 17 Sam Sneddon [:gsnedders] 2021-01-21 15:17:45 PST
(In reply to fantasai from comment #15)
> Isolation is definitely the correct thing to do here. I would say in
> general, if there's anywhere in your UI where you're presenting
> user-generated strings inline with other content, you want to isolate.
> 
> I'm a little unsure why you don't want dir=auto here, though. If you're
> dealing with RTL strings, wouldn't you want them handled as RTL?

Well, how do we expect a JS string such as "\u0633\u06cc\u0628\u002e" to get rendered in the console? I would expect "سیب." rather than ".سیب" as JS is normally treated as a LTR language?

Likewise, I'd expect "<span>\u0633\u06cc\u0628\u002e</span>" to render as "<span>سیب.</span>" (i.e., LTR, which admittedly dir=auto will compute to). Having the two strings put the bidi neutral character in a different place seems bad, as it becomes ambiguous as to what the value of the string is?

Like, is there any case where we'd want to render a JS string as RTL? That seems like that goes against the natural direction of JS?
Comment 18 Ebrahim Byagowi 2021-01-21 15:25:03 PST
> I'm a little unsure why you don't want dir=auto here, though. If you're dealing with RTL strings, wouldn't you want them handled as RTL?

No, I expect I see what I've entered on evaluation line just like other browser and what I've entered at the first place, actually regex shows the issue just better.

https://imgur.com/a/7PWC0xA

Something like

> /سشسیشسی/g
< g/سشسیشسی/

Happens if dir=auto is used here and this is why it would be unexpected.
Comment 19 Radar WebKit Bug Importer 2021-01-21 16:03:27 PST
<rdar://problem/73473434>
Comment 20 EWS 2021-01-21 17:24:51 PST
Committed r271726: <https://trac.webkit.org/changeset/271726>

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