1554209 - [RTL] Missing padding between text and accessibility icon

Status: VERIFIED FIXED

(DevTools :: Accessibility Tools, defect)

Description

[Itiel]

Attachment: Screenshot

See attached.

The problem is here:
https://searchfox.org/mozilla-central/source/devtools/client/accessibility/accessibility.css#233
Changing it to margin-inline-end: 12px; fixes the issue.

Comment 1

[Itiel]

:yzen, can this bug be set as good-first-bug and mentored by someone please? (not for me though)

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Itiel from comment #1)

:yzen, can this bug be set as good-first-bug and mentored by someone please? (not for me though)

Yes absolutely,

Looks like our padding (or margin) styling is not RTL proof for the icon.

Comment 3

[saijatin28]

Hey! I would love to work on this issue. To start off , how do i replicate this ?

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

hi @saijatin28, here are the steps to see the issue:

• Make sure you have a version of firefox that is RTL (e.g. arabic)
• Open developer tools (Tools -> Web Developer -> Toggle Tools)
• Open accessibility tab
• Notice the icon and text do not have any space between each other. It is likely because we are either using padding or margin with left/right rules instead of inline-start/inline-end.

Comment 5

[Itiel]

(In reply to Yura Zenevich [:yzen] from comment #4)

• Make sure you have a version of firefox that is RTL (e.g. arabic)

Or, in about:config change intl.uidirection to 1 and restart Firefox.

Comment 6

[saijatin28]

Attachment: Accessibilty icon

Hi! I have fixed this issue. Please have a look at the attachment.
Could you please guide me on how to push the code?

Comment 7

[saijatin28]

Attachment: accessibility_icon.png

Comment 8

[saijatin28]

Attachment: Screenshot from 2019-06-22 23-41-40.png

Comment 9

[Itiel]

(In reply to saijatin28 from comment #6)

Hi! I have fixed this issue. Please have a look at the attachment.
Could you please guide me on how to push the code?

https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#setting-up-arcanist
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#submitting-patches

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to saijatin28 from comment #6)

Created attachment 9073383 [details] [diff] [review]
Accessibilty icon

Hi! I have fixed this issue. Please have a look at the attachment.
Could you please guide me on how to push the code?

Yep, just like :Itiel suggested, you would need to install archanist and moz-phab to push to phabricator.
General guidelines can be found here too: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 11

[saijatin28]

Attachment: Added spacing to Accessibility icon in RTL Firefox builds.

Before this change, the accessibility icon was too close to the text beside it
on the RTL versions of Firefox.
Now a margin has been added to create sufficient space between the icon and
the text.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b587e6d83d55
Added spacing to Accessibility icon in RTL Firefox builds. r=yzen,gl

Comment 13

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/b587e6d83d55

Comment 14

[Itiel]

Fixed on latest Nightly.