Closed Bug 1013714 Opened 6 years ago Closed 5 years ago

Preferences data choices panel: weird focus ring on 'learn more' link

Categories

(Firefox :: Preferences, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 40
Tracking Status
firefox32 --- wontfix
firefox38 --- verified
firefox39 --- verified
firefox40 --- verified

People

(Reporter: soeren.hentzschel, Assigned: 526avijit, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(3 files, 1 obsolete file)

No special in-content preferences issue, the old preferences dialog has the same behaviour, but it looks really awful so please improve this with the new in-content-preferences. See the attached screenshot.
Blocks: 738796
Confirming on latest Nightly 32.0a1 (buildID: 20140609030202) that there is a weird focus ring on 'learn more' link that looks quite bad and needs to be fixed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][lang=css]
Mentor: jaws
I know which part of CSS is causing the problems by using Inspector Tool. But I am unable to figure out the CSS files. I know the CSS files are chrome://global/skin/global.css and chrome://global/skin/in-content/common.css
But I don't know where to look up these files.
(In reply to Shubham Jindal from comment #2)
> I know which part of CSS is causing the problems by using Inspector Tool.
> But I am unable to figure out the CSS files. I know the CSS files are
> chrome://global/skin/global.css and
> chrome://global/skin/in-content/common.css
> But I don't know where to look up these files.

Here's the file you need to edit : http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#364
You shouldn't edit global.css, because it's a global stylesheet for all xul apps, and that focusring is wanted on other xul apps.

But since you're removing the focus ring, you should re-add a focus state :
color: #ff9500;
text-decoration: underline;
In order to remove the focusring, I will have to add a style in http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css

.text-link:-moz-focusring{
  box-shadow:0px 0px 0px;
}
Is there any other way to remove the moz-focusring?As I am unable to find the rule of moz-focusring in common.inc.css which is displaying the ring around the text link.
Ignore my previous comment.:)

I finally figured out that the required CSS file is http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/in-content/common.css#63 and I have to remove the focusring from there.
To change the focus state, should I change the color as mentioned by you on text-link:hover or on text-link:hover:active or on just text-link in http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#364
(In reply to Shubham Jindal from comment #5)
> Ignore my previous comment.:)
> 
> I finally figured out that the required CSS file is
> http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/in-
> content/common.css#63 and I have to remove the focusring from there.
> To change the focus state, should I change the color as mentioned by you on
> text-link:hover or on text-link:hover:active or on just text-link in
> http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-
> content/common.inc.css#364
You can replace this :

html|a:-moz-focusring,
xul|*.text-link:-moz-focusring,
xul|*.inline-link:-moz-focusring {
  outline-width: 0;
  box-shadow: @focusRingShadow@;
}

with this :

html|a:-moz-focusring,
xul|*.text-link:-moz-focusring,
xul|*.inline-link:-moz-focusring {
  color: #ff9500;
  text-decoration: underline;
}
Shubham, are you still working on this?
Flags: needinfo?(shubhamjindal18)
Hi jaws,
Actually my Mac Trackpad stopped working. It will get fixed in a day or two. I will add a patch as soon as possible.
Flags: needinfo?(shubhamjindal18)
Hi Shubham, did you get your trackpad working?
Flags: needinfo?(shubhamjindal18)
could i work on this bug?
[i have been able to comprehend about the required fixes needed for the bug from the above conversation]
Flags: needinfo?(jaws)
(In reply to Avijit Gupta from comment #10)
> could i work on this bug?
> [i have been able to comprehend about the required fixes needed for the bug
> from the above conversation]

Yes, you may work on the bug. Thank you for your interest.
Flags: needinfo?(jaws)
> 
> html|a:-moz-focusring,
> xul|*.text-link:-moz-focusring,
> xul|*.inline-link:-moz-focusring {
>   outline-width: 0;
>   box-shadow: @focusRingShadow@;
> }
> 
> with this :
> 
> html|a:-moz-focusring,
> xul|*.text-link:-moz-focusring,
> xul|*.inline-link:-moz-focusring {
>   color: #ff9500;
>   text-decoration: underline;
> }

as per your comment #6 , i have made those changes here: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/in-content/common.css#80-81

are any other changes required for fixing this bug?
if not, then i am ready to submit a patch for the same.
Flags: needinfo?(ntim007)
(In reply to Avijit Gupta from comment #12)
> > 
> > html|a:-moz-focusring,
> > xul|*.text-link:-moz-focusring,
> > xul|*.inline-link:-moz-focusring {
> >   outline-width: 0;
> >   box-shadow: @focusRingShadow@;
> > }
> > 
> > with this :
> > 
> > html|a:-moz-focusring,
> > xul|*.text-link:-moz-focusring,
> > xul|*.inline-link:-moz-focusring {
> >   color: #ff9500;
> >   text-decoration: underline;
> > }
> 
> as per your comment #6 , i have made those changes here:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/in-
> content/common.css#80-81
> 
> are any other changes required for fixing this bug?
> if not, then i am ready to submit a patch for the same.
No more changes are needed.
Flags: needinfo?(ntim007)
Attachment #8557796 - Flags: review?(jaws)
Comment on attachment 8557796 [details] [diff] [review]
Patch - modified content.css `-moz-focuring` css rules

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

Thanks, this looks good. Did you remove the final line ending in this file? We should keep that around, as some tools complain when it is missing.
Attachment #8557796 - Flags: review?(jaws) → review+
Assignee: nobody → 526avijitgupta
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/3090906f8f4f
Keywords: checkin-needed
Whiteboard: [good first bug][lang=css] → [good first bug][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3090906f8f4f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=css][fixed-in-fx-team] → [good first bug][lang=css]
Target Milestone: --- → Firefox 38
The old focusring is still there on OSX, it seems to need box-shadow: none; to make it vanish.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Followup patchSplinter Review
The patch that landed only added new focusring styles, but didn't remove the old focusring styles, this patch does it.
Attachment #8591271 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8591271 [details] [diff] [review]
Followup patch

I mean, honestly? This doesn't really make sense to me. This is the global XUL link focusring, which is defined in global.css - if we think it's "weird" or needs fixing, we should have changed it there. I also don't think the active-like orange here makes sense for focus styling, and don't understand why we don't just use the same styling focused content links use (dotted borders, like on Windows).

For now, I guess if this is the intent of the bug then this patch accomplishes it, and I don't want to hold that up, but please file a followup and needinfo :shorlander .
Attachment #8591271 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #21)
> Comment on attachment 8591271 [details] [diff] [review]
> Followup patch
> 
> I mean, honestly? This doesn't really make sense to me. This is the global
> XUL link focusring, which is defined in global.css - if we think it's
> "weird" or needs fixing, we should have changed it there. I also don't think
> the active-like orange here makes sense for focus styling, and don't
> understand why we don't just use the same styling focused content links use
> (dotted borders, like on Windows).
The intent of Project Chameleon was to move away from the native styling, so native styles look awkward here. I agree that the orange color doesn't make much sense though.

> For now, I guess if this is the intent of the bug then this patch
> accomplishes it, and I don't want to hold that up, but please file a
> followup and needinfo :shorlander .
Filed bug 1153600
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/6e88231f30ba
Keywords: checkin-needed
Whiteboard: [good first bug][lang=css] → [good first bug][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6e88231f30ba
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=css][fixed-in-fx-team] → [good first bug][lang=css]
Target Milestone: Firefox 38 → Firefox 40
Comment on attachment 8591271 [details] [diff] [review]
Followup patch

Approval Request Comment
[Feature/regressing bug #]: incontent prefs
[User impact if declined]: user will see strange focusring on links inside the in content prefs
[Describe test coverage new/current, TreeHerder]: on m-c
[Risks and why]: low, css fix
[String/UUID change made/needed]: none
Attachment #8591271 - Flags: approval-mozilla-beta?
Attachment #8591271 - Flags: approval-mozilla-aurora?
Comment on attachment 8591271 [details] [diff] [review]
Followup patch

Should be in 38 beta 5.
Attachment #8591271 - Flags: approval-mozilla-beta?
Attachment #8591271 - Flags: approval-mozilla-beta+
Attachment #8591271 - Flags: approval-mozilla-aurora?
Attachment #8591271 - Flags: approval-mozilla-aurora+
Verified fixed using latest Nightly 40.0a1 (buildID: 20150427030207), latest Aurora 39.0a2 (buildID: 20150427004010) and Firefox 38 Beta 8 (buildID: 20150426174329).
You need to log in before you can comment on or make changes to this bug.