Closed Bug 1305007 Opened 8 years ago Closed 8 years ago

RTL direction has no effect on the inspector after HTML migration

Categories

(DevTools :: Inspector, defect, P1)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox52 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Iteration:
52.1 - Oct 3
Tracking Flags:
Tracking Status
firefox52 --- verified

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file, 1 obsolete file)

Bug 1305007 - set dir attribute value only if document.hasAttribute("dir");
58 bytes, text/x-review-board-request
bgrins
: review+
Details 
Regression from bug 1262443.

STRs:
- use an RTL build or use the Force RTL addon (https://addons.mozilla.org/en-us/firefox/addon/force-rtl/)
- open inspector

AR: All the inspector content is displayed in LTR. 
ER: Inspector should be respect the current locale direction
Blocks: 1262443
Whiteboard: [devtools-html] [triage]
Blocks: devtools-html-phase2
Flags: qe-verify?
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 52.1 - Oct 3
Priority: -- → P1
Whiteboard: [devtools-html] [triage] → [devtools-html]
(This first patch is more a feedback request than a review request, but mozreview doesn't support asking for feedback so ...)

In order :dir(rtl) selectors to be effective in HTML documents, we need to set the proper value for the "dir" attribute on the document's body.

The memory panel is currently using a the localized string "locale.dir" from global.dtd, but we can't rely on DTDs for devtools HTML. So I propose to add a helper on the toolbox that gets the computed value of "direction" on the topmost document. Panels are then responsible to get this value and set the proper dir attribute on their main container.

This will make testing RTL a bit more painful, as switching to RTL dynamically (using the addon ForceRTL for instance) will not have an immediate effect on devtools HTML panels.
Flags: qe-verify? → qe-verify+
QA Contact: cristian.comorasu
ni? in case you miss the review request. This new patch uses one of the approaches mentioned during standup. In the end I think it'd be nice to keep this in the toolbox and limit the impact on the tools. 

Could improve the current patch by creating a map of panel's iframes to avoid fetching them everytime a direction pref changes.

NB: I prefer to listen to all intl.uidirection pref changes and read the computed value of the direction rather than just reading the pref value. For instance, in a en-US browser, setting intl.uidirection.en and intl.uidirection.en-US will both have an impact on the current direction, so I think it could be tricky to know if a pref change actually has an impact without checking the computed value. The downside, as you'll see in the implementation is that we have to wait for an event loop before reading the computed value.
Flags: needinfo?(bgrinstead)
Comment on attachment 8794255 [details]
Bug 1305007 - set dir attribute value only if document.hasAttribute("dir");

https://reviewboard.mozilla.org/r/80800/#review79514

::: devtools/client/framework/toolbox.js:1278
(Diff revision 2)
>        // for a promise return value.
>        let built = definition.build(iframe.contentWindow, this);
> +
> +      // Set the dir attribute on the document element of the loading panel.
> +      let docEl = iframe.contentWindow.document.documentElement;
> +      docEl.setAttribute("dir", this._isRtl() ? "rtl" : "ltr");

Would these two lines be enough?

That is, do we want/need to support dynamic pref changes here or only through ForceRTL?  I believe if we correctly set the dir attribute from the start, then it will work in both a build of firefox in an rtl dir and with ForceRTL.
Attachment #8794255 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #5)
> Comment on attachment 8794255 [details]
> Bug 1305007 - RTL: update dir attribute on each panel documentElement when
> direction changes;
> 
> https://reviewboard.mozilla.org/r/80800/#review79514
> 
> ::: devtools/client/framework/toolbox.js:1278
> (Diff revision 2)
> >        // for a promise return value.
> >        let built = definition.build(iframe.contentWindow, this);
> > +
> > +      // Set the dir attribute on the document element of the loading panel.
> > +      let docEl = iframe.contentWindow.document.documentElement;
> > +      docEl.setAttribute("dir", this._isRtl() ? "rtl" : "ltr");
> 
> Would these two lines be enough?
> 
> That is, do we want/need to support dynamic pref changes here or only
> through ForceRTL?  I believe if we correctly set the dir attribute from the
> start, then it will work in both a build of firefox in an rtl dir and with
> ForceRTL.

It's not enough for ForceRTL, but I didn't have time to investigate why. Can take a look next week.
(NB: I'd like dynamic pref change to work though, this is the main way I personally test RTL)
So I spent some time looking at ForceRTL, and setting the dir on the document is not enough for two reasons:
- forceRTL only explores sub-documents of "browser" nodes [1]
- forceRTL skips half of the nodes when walking the tree [2]

[1] Opened https://github.com/mikedeboer/ForceRTL/issues/7
[2] Opened https://github.com/mikedeboer/ForceRTL/issues/5 + PR at Opened https://github.com/mikedeboer/ForceRTL/pull/6
Comment on attachment 8794255 [details]
Bug 1305007 - set dir attribute value only if document.hasAttribute("dir");

https://reviewboard.mozilla.org/r/80800/#review79720

As discussed, let's only handle the dir attribute on startup and deal with pref changes later on if we decide to

::: devtools/client/framework/toolbox.js:1278
(Diff revision 3)
>        // for a promise return value.
>        let built = definition.build(iframe.contentWindow, this);
> +
> +      // Set the dir attribute on the document element of the loading panel.
> +      let docEl = iframe.contentWindow.document.documentElement;
> +      docEl.setAttribute("dir", this._isRtl() ? "rtl" : "ltr");

Let's only set the dir attribute if it's a XUL doc
Attachment #8794255 - Flags: review?(bgrinstead)
I think we settled on a plan for this in our discussion, so clearing ni
Flags: needinfo?(bgrinstead)
Comment on attachment 8794255 [details]
Bug 1305007 - set dir attribute value only if document.hasAttribute("dir");

https://reviewboard.mozilla.org/r/80800/#review79846

Works for me, thanks
Attachment #8794255 - Flags: review?(bgrinstead) → review+
Thanks for the review! Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ae5b4803321

For the record https://github.com/mikedeboer/ForceRTL/pull/6 has been merged and https://github.com/mikedeboer/ForceRTL/pull/8 has been opened to fix "forceRTL only explores sub-documents of "browser" nodes".

Until both are merged and released with a new version of ForceRTL, we will still be able to test RTL mode by:
- changing direction
- closing devtools
- reopening devtools

After the new ForceRTL add-on is released, devtools will be automatically updated when changing the direction. 

In case anyone is setting the intl.uidirection pref in any other way, devtools will still have to be restarted, but that should not be a frequent use case.

Will need to update https://wiki.mozilla.org/DevTools/CSSTips#Localization with information mentioned above.
Comment on attachment 8795321 [details]
Bug 1305007 - Fix RTL in inspector styles after migration to HTML;

https://reviewboard.mozilla.org/r/81410/#review80006

::: devtools/client/shared/components/sidebar-toggle.css:11
(Diff revision 1)
>  .sidebar-toggle {
>    display: block;
>  }
>  
> -.sidebar-toggle:-moz-locale-dir(ltr)::before,
> -.sidebar-toggle.pane-collapsed:-moz-locale-dir(rtl)::before {
> +.sidebar-toggle::before,
> +.sidebar-toggle.pane-collapsed:-moz-locale-dir(rtl)::before,

I don't think we use the sidebar toggle React comp in other places than the inspector, so I think it should be safe to remove :-moz-locale-dir here and below.

::: devtools/client/shared/components/sidebar-toggle.css:17
(Diff revision 1)
> +.sidebar-toggle.pane-collapsed:dir(rtl)::before {
>    background-image: var(--theme-pane-collapse-image);
>  }
>  
> -.sidebar-toggle.pane-collapsed:-moz-locale-dir(ltr)::before,
> -.sidebar-toggle:-moz-locale-dir(rtl)::before {
> +.sidebar-toggle.pane-collapsed::before,
> +.sidebar-toggle:-moz-locale-dir(rtl)::before,

Same here.

::: devtools/client/shared/components/sidebar-toggle.css:31
(Diff revision 1)
>      transform: rotate(90deg);
>    }
>  
>    /* Since RTL swaps the used images, we need to flip them
>       the other way round */
> -  .sidebar-toggle:-moz-locale-dir(rtl)::before {
> +  .sidebar-toggle:-moz-locale-dir(rtl)::before,

Same here.

::: devtools/client/themes/rules.css:328
(Diff revision 1)
>  .ruleview-expander {
>    vertical-align: middle;
>    display: inline-block;
>  }
>  
> -.ruleview-expander.theme-twisty:-moz-locale-dir(rtl) {
> +.ruleview-expander.theme-twisty:-moz-locale-dir(rtl),

You can remove :-moz-locale-dir here, since the rule view is only used within the inspector which is a HTML doc
Comment on attachment 8795321 [details]
Bug 1305007 - Fix RTL in inspector styles after migration to HTML;

https://reviewboard.mozilla.org/r/81410/#review80108
Attachment #8795321 - Flags: review?(ntim.bugs) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/2afca4399441
RTL: set dir attr. when loading devtools HTML document;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/45513d6773f0
Fix RTL in inspector styles after migration to HTML;r=ntim
(In reply to Pulsebot from comment #21)
> Pushed by jdescottes@mozilla.com:
> https://hg.mozilla.org/integration/fx-team/rev/a15b46a9e102
> fix jetpack tests;r=bustage

The addon SDK creates devtools panels in 2 steps :
1. loads "about:blank" as the panel URL
2. when build() is being called by the toolbox, the sdk updates the panel's iframe URL to a new value

I don't know how the toolbox could plug itself on any event indicating that we reached the "final" document.
IMO we should set the dir attribute as soon as possible (even before calling build() on the panel). 

If the panel is using a custom implementation and changes the document after the iframe has been loaded, then the panel should be responsible for restoring a correct dir attribute value on the document. We could add a new API on the toolbox to get the correct "direction" value to use. 

I will push a cleanup for review.
Keywords: leave-open
Attachment #8795321 - Attachment is obsolete: true
Comment on attachment 8794255 [details]
Bug 1305007 - set dir attribute value only if document.hasAttribute("dir");

Uh, mozreview carried over your r+ for this new patch. Flagging for review.
Attachment #8794255 - Flags: review+ → review?(bgrinstead)
Comment on attachment 8794255 [details]
Bug 1305007 - set dir attribute value only if document.hasAttribute("dir");

https://reviewboard.mozilla.org/r/80800/#review80800

This looks good to me
Attachment #8794255 - Flags: review?(bgrinstead) → review+
Keywords: leave-open
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/5bf0ee1a10f4
set dir attribute value only if document.hasAttribute("dir");r=bgrins
https://hg.mozilla.org/mozilla-central/rev/5bf0ee1a10f4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Does this patch affect to new Debugger and DOM?
Flags: needinfo?(jdescottes)
(In reply to magicp from comment #30)
> Does this patch affect to new Debugger and DOM?

No, because both of those have no dir="" attribute.
Filed bug 1306840 for the DOM panel.
Filed https://github.com/devtools-html/debugger.html/issues/852 for the new debugger.
Flags: needinfo?(jdescottes)
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #31)

> No, because both of those have no dir="" attribute.
> Filed bug 1306840 for the DOM panel.
> Filed https://github.com/devtools-html/debugger.html/issues/852 for the new
> debugger.

Hi Tim, Thanks!
Depends on: 1306942
Depends on: 1306986
I reproduced this bug using Fx 52.0a1, build ID: 20160923030450, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 52.0a1, build ID: 20161003030438, on Windows 10 x64, mac OS X 10.11 and Ubuntu 14.04 LTS.
Cheers!
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: