Bug 171954 - Web Inspector: RTL: "Elements > Styles - Rules" rule and media query headers should be left aligned
Summary: Web Inspector: RTL: "Elements > Styles - Rules" rule and media query headers ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-10 16:32 PDT by Nikita Vasilyev
Modified: 2017-05-11 13:52 PDT (History)
5 users (show)

See Also:


Attachments
[Image] Bug (337.10 KB, image/png)
2017-05-10 16:32 PDT, Nikita Vasilyev
no flags Details
Patch (6.44 KB, patch)
2017-05-10 17:54 PDT, Nikita Vasilyev
mattbaker: review+
Details | Formatted Diff | Diff
[Image] With patch applied (405.68 KB, image/png)
2017-05-10 17:56 PDT, Nikita Vasilyev
no flags Details
[Animated GIF] Focusing on selector without dir=ltr (44.68 KB, image/gif)
2017-05-10 18:54 PDT, Nikita Vasilyev
no flags Details
Patch (6.59 KB, patch)
2017-05-11 11:23 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2017-05-10 16:32:12 PDT
Created attachment 309658 [details]
[Image] Bug

rdar://problem/31961976

Since the CSS source is strongly LTR, it doesn't make sense to flip CSS selectors.
Comment 1 BJ Burg 2017-05-10 16:58:51 PDT
<rdar://problem/31961976>
Comment 2 Nikita Vasilyev 2017-05-10 17:54:53 PDT
Created attachment 309665 [details]
Patch
Comment 3 Nikita Vasilyev 2017-05-10 17:56:10 PDT
Created attachment 309666 [details]
[Image] With patch applied
Comment 4 Matt Baker 2017-05-10 18:49:31 PDT
Comment on attachment 309665 [details]
Patch

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

r=me, with comment

> Source/WebInspectorUI/UserInterface/Views/StyleDetailsPanel.js:38
> +        this.element.dir = "ltr";

I don't understand why this is needed. Commenting it out doesn't break anything, as far as I can tell.
Comment 5 Nikita Vasilyev 2017-05-10 18:54:06 PDT
Created attachment 309674 [details]
[Animated GIF] Focusing on selector without dir=ltr

(In reply to Matt Baker from comment #4)
> Comment on attachment 309665 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309665&action=review
> 
> r=me, with comment
> 
> > Source/WebInspectorUI/UserInterface/Views/StyleDetailsPanel.js:38
> > +        this.element.dir = "ltr";
> 
> I don't understand why this is needed. Commenting it out doesn't break
> anything, as far as I can tell.

Try editing a selector. I left a comment in the changelog.
Comment 6 Matt Baker 2017-05-10 21:31:29 PDT
Comment on attachment 309665 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/StyleDetailsPanel.js:38
>>> +        this.element.dir = "ltr";
>> 
>> I don't understand why this is needed. Commenting it out doesn't break anything, as far as I can tell.
> 
> Try editing a selector. I left a comment in the changelog.

Ah, okay. I forgot selectors could be edited!

I think the direction only needs to be set for the selector input (since WebInspector.CodeMirrorEditor already forces dir="ltr"). Maybe it should move to CSSStyleDeclarationSection.js:66, where the <textarea> is created:

if (this.selectorEditable) {
    this._selectorInput = this._headerElement.createChild("textarea");
    this._selectorInput.setAttribute("dir", "ltr");
    this._selectorInput.spellcheck = false;
    ...
}
Comment 7 Nikita Vasilyev 2017-05-11 11:23:51 PDT
Created attachment 309729 [details]
Patch

(In reply to Matt Baker from comment #6)
> Comment on attachment 309665 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309665&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Views/StyleDetailsPanel.js:38
> >>> +        this.element.dir = "ltr";
> >> 
> >> I don't understand why this is needed. Commenting it out doesn't break anything, as far as I can tell.
> > 
> > Try editing a selector. I left a comment in the changelog.
> 
> Ah, okay. I forgot selectors could be edited!
> 
> I think the direction only needs to be set for the selector input (since
> WebInspector.CodeMirrorEditor already forces dir="ltr"). Maybe it should
> move to CSSStyleDeclarationSection.js:66, where the <textarea> is created:
> 
> if (this.selectorEditable) {
>     this._selectorInput = this._headerElement.createChild("textarea");
>     this._selectorInput.setAttribute("dir", "ltr");
>     this._selectorInput.spellcheck = false;
>     ...
> }

I guess it's unlikely that we will add any inputs or textareas to StyleDetailsPanel any time soon, so it's fine with me.
Comment 8 Nikita Vasilyev 2017-05-11 11:25:49 PDT
Comment on attachment 309729 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:68
>              this._selectorInput.spellcheck = false;
> +            this._selectorInput.dir = "ltr";
>              this._selectorInput.tabIndex = -1;

I used `dir = "ltr"` instead of setAttribute because it's shorter. Also, we already use the setter syntax for spellcheck and tabIndex attributes.
Comment 9 Matt Baker 2017-05-11 11:51:37 PDT
Looks good!
Comment 10 WebKit Commit Bot 2017-05-11 13:52:22 PDT
Comment on attachment 309729 [details]
Patch

Clearing flags on attachment: 309729

Committed r216692: <http://trac.webkit.org/changeset/216692>
Comment 11 WebKit Commit Bot 2017-05-11 13:52:24 PDT
All reviewed patches have been landed.  Closing bug.