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

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Developer Tools: Inspector
P3
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: tomer, Assigned: beekill, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 53
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 months ago
Created attachment 8822493 [details]
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.

Updated

5 months ago
Summary: [RTL] Links to MDN should appear as LTR if article is not localized → [RTL] MDN articles should appear as LTR if they're not localized
(Reporter)

Updated

5 months ago
Summary: [RTL] MDN articles should appear as LTR if they're not localized → [RTL] MDN article snippets should appear as LTR if they're not localized
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
Mentor: jdescottes@mozilla.com
Keywords: good-first-bug
Priority: -- → P3
(Assignee)

Comment 2

4 months ago
Hi, can I try on this bug?

Comment 3

4 months ago
Towkir, are you interested in this bug?
Flags: needinfo?(3ugzilla)
: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.
Assignee: nobody → nnn_bikiu0707

Updated

4 months ago
Flags: needinfo?(3ugzilla)
(Assignee)

Comment 5

4 months ago
Created attachment 8827313 [details] [diff] [review]
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.
Attachment #8827313 - Flags: review?(jdescottes)
Attachment #8827313 - Flags: feedback?
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
Attachment #8827313 - Flags: review?(jdescottes)
Attachment #8827313 - Flags: feedback?
Attachment #8827313 - Flags: feedback+
(Assignee)

Comment 7

4 months ago
Created attachment 8827462 [details] [diff] [review]
add_ltr_mdndocswidget.patch

No problem. I added some lines to the MdnDocsWidget.js. Let me know what you think.
Attachment #8827313 - Attachment is obsolete: true
Attachment #8827462 - Flags: feedback?(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.
Attachment #8827462 - Flags: feedback?(jdescottes) → review+
Created attachment 8827472 [details] [diff] [review]
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!
Attachment #8827462 - Attachment is obsolete: true
Attachment #8827472 - Flags: review+
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.
(Assignee)

Comment 11

4 months ago
(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

4 months ago
(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.
Keywords: checkin-needed
(Assignee)

Comment 13

4 months ago
Thank you guys :)

Comment 14

4 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be42c91eaa2
Set attribute dir to ltr to fix MDN article snippets appear in RTL. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4be42c91eaa2
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.