Closed Bug 1173036 Opened 9 years ago Closed 9 years ago

Change Loop's RTL attributes to be consistently set on the html element

Categories

(Hello (Loop) :: Client, defect, P1)

Product:
Hello (Loop)
Component:
Client
Type:
defect
Points:
2

Tracking

(firefox41 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla41
Iteration:
41.3 - Jun 29
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

Change Loop's RTL attributes to be consistently set on the html element.
17.72 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review 
When we set up the RTL mode items, we happened to make it so that the standalone has dir="rtl" set on the <html> element, and the panel/conversation window have it set on the <body> element.

This is causing issues with writing css for the standalone UI, as the dir attribute is in a different place.

From what I've seen searching around, putting it on the html element is the right thing to do, as browser may also then apply it to things like the title element.
Summary: Change Loop's RTL attributes to be consistently set on the html attribute → Change Loop's RTL attributes to be consistently set on the html element
This updates us to use the html element in all cases. I've also gone for making the css consistent wrt quotes.

Additionally, as an aid for development, I've added an "RTL Mode" button onto the ui-showcase. It automatically reloads the page because of the way we create our framed elements, but this shouldn't hurt developers too much - given the benefit of easily being able to see the different layouts.
Attachment #8617582 - Flags: review?(mdeboer)
Assignee: nobody → standard8
Iteration: --- → 41.3 - Jun 29
Points: --- → 2
Rank: 18
Flags: qe-verify-
Flags: firefox-backlog+
Priority: -- → P1
Comment on attachment 8617582 [details] [diff] [review]
Change Loop's RTL attributes to be consistently set on the html element.

Review of attachment 8617582 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with my 'attributes comment' addressed. The MutationObserver thing is probably a mad idea.

::: browser/components/loop/content/js/conversation.jsx
@@ +163,5 @@
>        dispatcher={dispatcher}
>        mozLoop={navigator.mozLoop} />, document.querySelector("#main"));
>  
> +    document.documentElement.lang = mozL10n.getLanguage();
> +    document.documentElement.dir = mozL10n.getDirection();

Why not keep this as attributes?

::: browser/components/loop/content/js/panel.jsx
@@ +1002,5 @@
>        mozLoop={navigator.mozLoop}
>        dispatcher={dispatcher} />, document.querySelector("#main"));
>  
> +    document.documentElement.lang = mozL10n.getLanguage();
> +    document.documentElement.dir = mozL10n.getDirection();

Same question as in conversation.jsx

::: browser/components/loop/ui/react-frame-component.js
@@ +68,5 @@
> +      // See also "ShowCase" in ui-showcase.jsx
> +      if (document.location.search === "?rtl=1") {
> +        doc.documentElement.lang = "ar";
> +        doc.documentElement.dir = "rtl";
> +      }

What we could also do, if sensible at all, is make these use `setAttribute` everywhere and add a MutationObserver for attribute changes in the iframes' parent document.

Prolly crazy idea.

Still, I'd like to see the attributes to remain proper DOM attributes.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +451,5 @@
>          <div className="showcase">
>            <header>
>              <h1>Loop UI Components Showcase</h1>
> +            <Checkbox label="RTL mode?" checked={this.state.rtlMode}
> +              onChange={this._onCheckboxChange}/>

nit: space before `/>`

I think we have the convention to call event handlers like `this.handleCheckboxChange`.
Attachment #8617582 - Flags: review?(mdeboer) → review+
Status: NEW → ASSIGNED
Looks like setAttribute is slightly better to keep at the moment due to browser support.

The MutationObserver is possibly a bit extra that I wanted to do at the moment, but I'll bear it in mind for a future tweak.
https://hg.mozilla.org/mozilla-central/rev/b971fbbbe37e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: