Links in in-content UI pages have two focus rings

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Themes
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: dao, Assigned: Ganesh Chaitanya Kale, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla55
All
Linux
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: [good first bug][lang=css])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 months ago
Links in in-content UI pages (such as "Learn more" in about:preferences#content) currently have two focus rings.

One from here:
https://hg.mozilla.org/mozilla-central/annotate/5069348353f8fc1121e632e3208da33900627214/toolkit/themes/linux/global/global.css#l282

And one from here:
https://hg.mozilla.org/mozilla-central/annotate/5069348353f8fc1121e632e3208da33900627214/toolkit/themes/linux/global/in-content/common.css#l93

We should remove the latter.

Comment 1

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

Comment 2

4 months ago
Thanks for the patch. You need to ask for review - just click the "details" link for the attachment and then set the review flag to "?" and enter dao+bmo@mozilla.com (who's also mentoring this).
Assignee: nobody → joshua.d.horwitz
Status: NEW → ASSIGNED

Updated

4 months ago
Attachment #8841233 - Flags: review?(dao+bmo)

Comment 3

4 months ago
Thank you very much, done!

Comment 4

4 months ago
Comment on attachment 8841233 [details] [diff] [review]
patch.diff

I just looked at the patch, and to make a long story short: global.css handle styles appearing everywhere in the ui, but we just have a problem with the in-content ui pages. In other words, you want to remove the style rule in toolkit/themes/linux/global/in-content/common.css instead.

Comment 5

4 months ago
Thank you, I have updated the patch.

Comment 6

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

Updated to remove from common.css instead of global.css
Attachment #8841233 - Attachment is obsolete: true
Attachment #8841233 - Flags: review?(dao+bmo)
Attachment #8841279 - Flags: review?(dao+bmo)

Comment 7

4 months ago
Comment on attachment 8841279 [details] [diff] [review]
patch.diff

diff --git a/toolkit/themes/linux/global/global.css b/toolkit/themes/linux/global/global.css
--- a/toolkit/themes/linux/global/global.css
+++ b/toolkit/themes/linux/global/global.css

Hmm, wrong patch uploaded?

Comment 8

4 months ago
Created attachment 8841281 [details] [diff] [review]
patch.diff
Attachment #8841279 - Attachment is obsolete: true
Attachment #8841279 - Flags: review?(dao+bmo)
Attachment #8841281 - Flags: review?(dao+bmo)

Comment 9

4 months ago
Sorry about that, added the wrong revision to the patch!  Coming from SVN and Git world.  Thank you for the hel
(Reporter)

Comment 10

4 months ago
Comment on attachment 8841281 [details] [diff] [review]
patch.diff

You seem to have committed the previous patch where you removed the rule from global.css, and this diff is against that commit so it appears to be adding that rule back. Instead the diff should be against mozilla-central tip and not touch global.css at all.
Attachment #8841281 - Flags: review?(dao+bmo)
(Reporter)

Comment 11

4 months ago
Joshua, can you make sense of my previous comment?
Flags: needinfo?(joshua.d.horwitz)

Comment 12

4 months ago
Thank you for the help, how can I rectify this and go back and make this correct change?
(Reporter)

Comment 13

4 months ago
(In reply to joshua.d.horwitz from comment #12)
> Thank you for the help, how can I rectify this and go back and make this
> correct change?

It depends on your hg workflow... How exactly did you create these patches in the first place?

In #introduction there are probably people who can help you with questions about hg too.
(Reporter)

Comment 14

4 months ago
Joshua, do you still want to work on this?
(Reporter)

Updated

4 months ago
Assignee: joshua.d.horwitz → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(joshua.d.horwitz)
(Reporter)

Updated

4 months ago
Attachment #8841281 - Attachment is obsolete: true
(Assignee)

Comment 15

3 months ago
(In reply to Dão Gottwald [::dao] from comment #14)
> Joshua, do you still want to work on this?

Hi Dao,

Can I start working on this, If so can assign it to me. I just did a setup of my Firefox with default branch. I would like to have input from you on how to work on the fix.


Regards,
Ganesh
(Assignee)

Comment 16

3 months ago
Created attachment 8849166 [details] [diff] [review]
Fix for the bug in Patch file Bug 1342370_Patch.patch

I have attached the patch with the fix, please review it and let me know if any changes are necessary.

Regards,
Ganesh
Attachment #8849166 - Flags: review?(stefanh)
Attachment #8849166 - Flags: review?(dao+bmo)
(Reporter)

Comment 17

3 months ago
Comment on attachment 8849166 [details] [diff] [review]
Fix for the bug in Patch file Bug 1342370_Patch.patch

Thanks!
Attachment #8849166 - Flags: review?(stefanh)
Attachment #8849166 - Flags: review?(dao+bmo)
Attachment #8849166 - Flags: review+
(Reporter)

Updated

3 months ago
Assignee: nobody → ganesh2583

Comment 18

3 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e399a0575a4
Remove focus ring from common.css as its already in global.css. r=dao

Comment 19

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2e399a0575a4
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox54: affected → wontfix
You need to log in before you can comment on or make changes to this bug.