1326265 - [RTL] MDN article snippets should appear as LTR if they're not localized

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Tomer Cohen :tomer]

Attachment: Screenshot

Steps to reproduce: 
a. Open inspector on an RTL Firefox build.
b. Right click on any rule and choose to open the MDN documentation from the context menu. 

Expected result:
Since these documents are not available on Hebrew locale, documentation should be displayed as LTR. 

We have a similar issue for the regular MDN; if the popup is an iframe, it might be more suitable to fix this issue on the MDN side.

Comment 1

[Julian Descottes [:jdescottes]]

Inspector bug triage, filter on CLIMBING SHOES.

For the moment we always use the en-US version of MDN for this popup, so it would make sense to force the content to use dir=ltr here. The popup content is created in CssDocsTooltip.js [1]. Adding the attribute "dir" set to "ltr" should fix it here. Should be a good first bug. 

To test RTL direction with a local build of devtools, the easiest way is to use the ForceRTL add on (https://addons.mozilla.org/en-US/firefox/addon/force-rtl/).

[1] http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/devtools/client/shared/widgets/tooltip/CssDocsTooltip.js#77-82

Comment 2

[Bao Quan [:beekill]]

Hi, can I try on this bug?

Comment 3

[Tim Nguyen :ntim]

Towkir, are you interested in this bug?

Comment 4

[Julian Descottes [:jdescottes]]

:ntim sorry to override you here, but I meant to reply to nnn_bikiu0707 first. 
They showed interest in this bug first, so it's fair to assign it accordingly :) 

nnn_bikiu0707: Thanks for wanting to fix this! I gave a few pointers in comment 1. If this is your first contribution, have a look at our contribution guides:
- https://developer.mozilla.org/en-US/docs/Tools/Contributing
- https://wiki.mozilla.org/DevTools/Hacking

You can reach us on IRC in the #devtools channel if you have any question.
Or simply comment on this bug, and if you have a question don't forget to set a "Need information" flag using the form below. This way you will be sure we get a notification about your comment.

Comment 5

[Bao Quan [:beekill]]

Attachment: fix_bug_1326265.patch

Hi :jdescottes], thank you for letting me work on this bug. I did what you mentioned in comment 1. Let me know if something needs changing.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8827313 [details] [diff] [review]
fix_bug_1326265.patch

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

That fixes the issue, thanks! 

I now think it would be better to do this in the constructor of MdnDocsWidget.js [1]
The reason is that we can "force" ltr because we use "https://developer.mozilla.org/en-US/docs/Web/CSS/" as the base URL for getting the content from MDN and this is all done in MdnDocsWidget. Can you also add a small comment explaining that rtl can be forced because we use the en-US version of MDN?

Sorry for changing my mind about this, but it should be easy to adapt your current patch! 

[1] http://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/MdnDocsWidget.js#244-279

Comment 7

[Bao Quan [:beekill]]

Attachment: add_ltr_mdndocswidget.patch

No problem. I added some lines to the MdnDocsWidget.js. Let me know what you think.

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8827462 [details] [diff] [review]
add_ltr_mdndocswidget.patch

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

Perfect thanks! I will submit your changeset to try, which is our continuous integration platform. 
This will run our test suite on a various set of platforms, to make sure the change didn't introduce unexpected regressions.

If the tests are ok we will then ask to a sheriff to land your changeset on one of our integration branches. It will then "ride the trains" from Nightly to release and will ship with Firefox 53.

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: add_ltr_mdndocswidget.amend.patch

I just added r=jdescottes to your commit message so that we can keep track of the reviewer after this lands!

Comment 10

[Julian Descottes [:jdescottes]]

Here is the link to the try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=646669e7f04e2d6ff9cdc8622b33079af57c624c

This page will be updated with test results as soon as the test suites are completed.

Comment 11

[Bao Quan [:beekill]]

(In reply to Julian Descottes [:jdescottes] from comment #10)
> Here is the link to the try run:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=646669e7f04e2d6ff9cdc8622b33079af57c624c
> 
> This page will be updated with test results as soon as the test suites are
> completed.

Thank you for your help. I looked at the page and it showed some tests failed. What should we do?

Comment 12

[Tim Nguyen :ntim]

(In reply to Bao Quan [:beekill95] from comment #11)
> (In reply to Julian Descottes [:jdescottes] from comment #10)
> > Here is the link to the try run:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=646669e7f04e2d6ff9cdc8622b33079af57c624c
> > 
> > This page will be updated with test results as soon as the test suites are
> > completed.
> 
> Thank you for your help. I looked at the page and it showed some tests
> failed. What should we do?

None of the failures are related to your patch. Congrats on your first patch :)

I'm going to set checkin-needed and it should land in Nightly in about 2 days.

Comment 13

[Bao Quan [:beekill]]

Thank you guys :)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be42c91eaa2
Set attribute dir to ltr to fix MDN article snippets appear in RTL. r=jdescottes

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4be42c91eaa2