Closed Bug 1554209 Opened 7 months ago Closed 5 months ago

[RTL] Missing padding between text and accessibility icon

Categories

(DevTools :: Accessibility Tools, defect)

defect
Not set

Tracking

(firefox69 verified)

VERIFIED FIXED
Firefox 69
Tracking Status
firefox69 --- verified

People

(Reporter: itiel_yn8, Assigned: saijatin28, Mentored)

Details

(Keywords: good-first-bug, rtl)

Attachments

(3 files, 2 obsolete files)

Attached image 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.

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

Flags: needinfo?(yzenevich)

(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.

Mentor: yzenevich
Flags: needinfo?(yzenevich)
Keywords: good-first-bug

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

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.

(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.

Attached patch Accessibilty icon (obsolete) — Splinter Review

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

Flags: needinfo?(yzenevich)
Attached patch accessibility_icon.png (obsolete) — Splinter Review
Attachment #9073383 - Attachment is obsolete: true
Attachment #9073384 - Attachment is obsolete: true

(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

(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

Flags: needinfo?(yzenevich)

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.

Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b587e6d83d55
Added spacing to Accessibility icon in RTL Firefox builds. r=yzen,gl
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
Assignee: nobody → saijatin28

Fixed on latest Nightly.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.