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

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Sören Hentzschel]

Attachment: preferences-advanced-4.png

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.

Comment 1

[Camelia Badau [:cbadau], Desktop QA]

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.

Comment 2

[Shubham Jindal [:shubham]]

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.

Comment 3

[Tim Nguyen :ntim]

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

Comment 4

[Shubham Jindal [:shubham]]

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.

Comment 5

[Shubham Jindal [:shubham]]

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

Comment 6

[Tim Nguyen :ntim]

(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;
}

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Shubham, are you still working on this?

Comment 8

[Shubham Jindal [:shubham]]

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.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Hi Shubham, did you get your trackpad working?

Comment 10

[Avijit Gupta]

could i work on this bug?
[i have been able to comprehend about the required fixes needed for the bug from the above conversation]

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 12

[Avijit Gupta]

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

Comment 13

[Tim Nguyen :ntim]

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

Comment 14

[Avijit Gupta]

Attachment: Patch - modified content.css `-moz-focuring` css rules

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for checkin

Comment 17

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/3090906f8f4f

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3090906f8f4f

Comment 19

[Tim Nguyen :ntim]

The old focusring is still there on OSX, it seems to need box-shadow: none; to make it vanish.

Comment 20

[Tim Nguyen :ntim]

Attachment: Followup patch

The patch that landed only added new focusring styles, but didn't remove the old focusring styles, this patch does it.

Comment 21

[:Gijs (he/him)]

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 .

Comment 22

[Tim Nguyen :ntim]

(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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/6e88231f30ba

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e88231f30ba

Comment 25

[Tim Nguyen :ntim]

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

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8591271 [details] [diff] [review]
Followup patch

Should be in 38 beta 5.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c2a34449bc99

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/48976876cdb9

Comment 29

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed using latest Nightly 40.0a1 (buildID: 20150427030207), latest Aurora 39.0a2 (buildID: 20150427004010) and Firefox 38 Beta 8 (buildID: 20150426174329).