pinstripe: non-native focus rings are applied for links in chrome

RESOLVED FIXED in mozilla25

Status

()

Toolkit
Themes
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mano, Assigned: stefanh)

Tracking

Trunk
mozilla25
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Created attachment 732679 [details]
example
Summary: pinstripe: non-native focus rings is applied for links in chrome → pinstripe: non-native focus rings are applied for links in chrome
Created attachment 732681 [details]
native ui

Updated

4 years ago
Component: Theme → Themes
Product: Firefox → Toolkit
(Assignee)

Comment 2

4 years ago
Created attachment 733034 [details] [diff] [review]
Use the standard focus ring shadow

I had to overrule layout/style/html.css because of the html links. I haven't tried to fix the xul - so (for example), the focus ring around the "Learn More" link in the Privacy pane is as stretched as usual.
Assignee: nobody → stefanh
Status: NEW → ASSIGNED
Attachment #733034 - Flags: review?(mano)
(Assignee)

Comment 3

4 years ago
Created attachment 733037 [details]
Screenshots
Why did you repeat the box-shadow rule in preferences?
(Assignee)

Comment 5

4 years ago
(In reply to Mano from comment #4)
> Why did you repeat the box-shadow rule in preferences?

You mean this?
+html|a.inline-link:-moz-focusring {
+  outline-width: 0;
+  box-shadow: @focusRingShadow@;
+}
+

The .inline-link is html and that's not covered by the rule in global.css (see also comment #2).
(Assignee)

Comment 6

4 years ago
That said, perhaps we should also style the html links in global.css... I'll have look tomorrow.
(Assignee)

Comment 7

4 years ago
So in mozilla-central/comm-central browser prefs is the only place where inline-link is used as a classname for an html:a. In all other places inline-link is as a classname for xul elements and it's also used together with text-link. So I don't see any reason to style inline-link in global.css. We could of course probably style html|a elements in global.css, but I'm not sure if we want that.

It appears that I was wrong when I said I needed to use the namespace declarations - it actually works by just doing

.inline-link:-moz-focusring {
  outline-width: 0;
  box-shadow: @focusRingShadow@;
}

So I could reduce some code if you want (feels a bit wrong, though).
(Assignee)

Comment 8

4 years ago
Comment on attachment 733034 [details] [diff] [review]
Use the standard focus ring shadow

Dao, perhaps you can look at this? I assume Mano is away/over-burdened.
Attachment #733034 - Flags: review?(mano) → review?(dao)
Comment on attachment 733034 [details] [diff] [review]
Use the standard focus ring shadow

Please file a follow up to cleanup the usage of links in the preferences dialog. I'm not sure why some links are "hardcoded" the way they are.
Attachment #733034 - Flags: review?(dao) → review+
(Assignee)

Comment 10

4 years ago
> Please file a follow up to cleanup the usage of links in the preferences
> dialog. I'm not sure why some links are "hardcoded" the way they are.

Filed bug 869078.
(Assignee)

Comment 11

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/bfa11f026529
Backed out for causing OSX mochitest-a11y to assert.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb837e2d738

https://tbpl.mozilla.org/php/getParsedLog.php?id=22643815&full=1&branch=mozilla-inbound#error0

12:16:24     INFO -  4157 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/hyperlink/test_general.xul | state bits should not be present in ID  'linkLabelWithValue' !
12:16:24     INFO -  ###!!! ASSERTION: UpdateLabelValue was called for wrong accessible!: 'xulLabel', file ../../../../accessible/src/base/nsAccessibilityService.cpp, line 491
(Assignee)

Comment 13

4 years ago
Hmm, I don't understand how the css changes could cause this... I do change the .text-link, though. I'll look at this in acouple of days.
(Assignee)

Comment 14

4 years ago
Looks like it's the box-shadow rule in global.css that causes the assertion...
(Assignee)

Comment 15

4 years ago
So, the assertion happens when the focustest are run for linkLabelWithValue. I have no idea what's going on except for that this could be related to layout differences (?).

I've pushed https://hg.mozilla.org/try/rev/c2f7a1c4a0ad, just to see if the assertion also happens on win/nix.
(Assignee)

Comment 16

4 years ago
> I've pushed https://hg.mozilla.org/try/rev/c2f7a1c4a0ad, just to see if the
> assertion also happens on win/nix.

Results are here: https://tbpl.mozilla.org/?tree=Try&rev=c2f7a1c4a0ad

As you can see, the assertion also happens on linux debug , but not on windows. Try builds/logs are here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/stefanh@inbox.com-c2f7a1c4a0ad/

CC:ing tbsaunde and surkov - perhaps they're able to see what's going on here.

Comment 17

4 years ago
(In reply to Stefan [:stefanh] from comment #15)
> So, the assertion happens when the focustest are run for linkLabelWithValue.
> I have no idea what's going on except for that this could be related to
> layout differences (?).

Sounds plausible. We assert when XUL label has an accessible but it's not XULLabelAccessible. XULLabelAccessible is created if nsIAccessibleProvider::accessibleType returns "XULText" which is implemented by XBL binding. Can that style affect somehow on XBL binding used for XUL label?
(Assignee)

Comment 18

4 years ago
(In reply to alexander :surkov from comment #17)
>  Can that style affect somehow on XBL binding used for XUL
> label?

Good question. It surely sounds wrong that a style rule should affect it... should I file a new bug for this?
(Assignee)

Comment 19

4 years ago
Since it's the .text-link binding we use we return "XULLink". I don't understand why we then call updateLabelValue in nsAccessibilityService.cpp.

Comment 20

4 years ago
(In reply to Stefan [:stefanh] from comment #18)
> (In reply to alexander :surkov from comment #17)
> >  Can that style affect somehow on XBL binding used for XUL
> > label?
> 
> Good question. It surely sounds wrong that a style rule should affect it...
> should I file a new bug for this?

I don't know. A question should be addressed to layout peer.

(In reply to Stefan [:stefanh] from comment #19)
> Since it's the .text-link binding we use we return "XULLink".

it shouldn't be .text-link, otherwise XULLabelAccessible was in use. Btw, which accessible class you get an assertion for?

> I don't
> understand why we then call updateLabelValue in nsAccessibilityService.cpp.

it's called whenever value of XUL label may be changed (@value was changed or binding was attached I guess, maybe something else).
(Assignee)

Comment 21

4 years ago
(In reply to alexander :surkov from comment #20)
> (In reply to Stefan [:stefanh] from comment #18)
> > (In reply to alexander :surkov from comment #17)
> > >  Can that style affect somehow on XBL binding used for XUL
> > > label?
> > 
> > Good question. It surely sounds wrong that a style rule should affect it...
> > should I file a new bug for this?
> 
> I don't know. A question should be addressed to layout peer.

OK, thanks. I'm not able to fix the assertion and the reason I asked is that in that case I could have changed the test to not fail because of the assertion. IOTW, I could have landed the patch and closed this bug.
> 
> (In reply to Stefan [:stefanh] from comment #19)
> > Since it's the .text-link binding we use we return "XULLink".
> 
> it shouldn't be .text-link, otherwise XULLabelAccessible was in use. Btw,
> which accessible class you get an assertion for?

The test that seems to cause the assertion has this label (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/hyperlink/test_general.xul#94):
<label id="linkLabelWithValue" value="Mozilla Foundation" class="text-link"
   href="http://www.mozilla.org/" />

Re the class - I'm at work atm, but I could try and debug it during the weekend if you think that might help.
(Assignee)

Comment 22

4 years ago
roc, do you have any suggestion on how to move forward here? Using box-shadow when focusing a .text-link label (with value set) seems to cause an assertion in nsAccessibilityService.cpp (updateLabelValue). This happens on both mac and linux (see comment #16 for the patch that was pushed to try).

Should I file new bug for the assertion and then add a 'SimpleTest.expectAssertions(0, 1);' to the test?
I'm not sure what the problem is but I bet Markus can figure it out :-)
Flags: needinfo?(mstange)
(In reply to alexander :surkov from comment #20)
> (In reply to Stefan [:stefanh] from comment #19)
> > Since it's the .text-link binding we use we return "XULLink".
> 
> it shouldn't be .text-link, otherwise XULLabelAccessible was in use.

Can you elaborate on this?
We're dealing with a label.text-link. The class "text-link" sets the binding text.xml#text-link on it, which returns the accessible type XULLink. So the label's accessible is not a XULLabelAccessible, but a XULLinkAccessible.
It seems to me that either the assertion is misplaced or that the text-link binding can't be used on labels.

It's strange that the box-shadow rule triggers this, but I suspect it just adds a call to nsTextBoxFrame::CalcDrawRect which wasn't there before (which calls UpdateLabelValue).
Flags: needinfo?(mstange)
Flags: needinfo?(surkov.alexander)

Comment 25

4 years ago
(In reply to Markus Stange from comment #24)

> We're dealing with a label.text-link. The class "text-link" sets the binding
> text.xml#text-link on it, which returns the accessible type XULLink. So the
> label's accessible is not a XULLabelAccessible, but a XULLinkAccessible.
> It seems to me that either the assertion is misplaced or that the text-link
> binding can't be used on labels.

it's misplaced. please file a bug that XULLinkAccessible should implement text interface similar to XULLabelAccessible and remove (comment out) that assertion.
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #25)
> (In reply to Markus Stange from comment #24)
> 
> > We're dealing with a label.text-link. The class "text-link" sets the binding
> > text.xml#text-link on it, which returns the accessible type XULLink. So the
> > label's accessible is not a XULLabelAccessible, but a XULLinkAccessible.
> > It seems to me that either the assertion is misplaced or that the text-link
> > binding can't be used on labels.
> 
> it's misplaced. please file a bug that XULLinkAccessible should implement
> text interface similar to XULLabelAccessible and remove (comment out) that
> assertion.

wouldn't it be better to use expectAssertions() in the test?

Comment 27

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #26)

> wouldn't it be better to use expectAssertions() in the test?

that works too, I think I would even prefer it
(Assignee)

Comment 28

4 years ago
(In reply to alexander :surkov from comment #25)
> (In reply to Markus Stange from comment #24)
> 
> > We're dealing with a label.text-link. The class "text-link" sets the binding
> > text.xml#text-link on it, which returns the accessible type XULLink. So the
> > label's accessible is not a XULLabelAccessible, but a XULLinkAccessible.
> > It seems to me that either the assertion is misplaced or that the text-link
> > binding can't be used on labels.
> 
> it's misplaced. please file a bug that XULLinkAccessible should implement
> text interface similar to XULLabelAccessible and remove (comment out) that
> assertion.

Filed bug 883672.
(Assignee)

Comment 29

4 years ago
Created attachment 769720 [details] [diff] [review]
Fix test_general.xul to expect 1 assertion on mac

Sorry for the delay, but here's comment #27 implemented
Attachment #769720 - Flags: review?(surkov.alexander)

Comment 30

4 years ago
Comment on attachment 769720 [details] [diff] [review]
Fix test_general.xul to expect 1 assertion on mac

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

r=me

::: accessible/tests/mochitest/hyperlink/test_general.xul
@@ +22,5 @@
>            src="hyperlink.js" />
>  
>    <script type="application/javascript">
>    <![CDATA[
> +    if (navigator.platform.startsWith("Mac"))

if (MAC)

should work here (defined in common.js)

@@ +23,5 @@
>  
>    <script type="application/javascript">
>    <![CDATA[
> +    if (navigator.platform.startsWith("Mac"))
> +      SimpleTest.expectAssertions(0, 1);

isn't it permanent failure, shouldn't you have expectAssertions(1, 1)?
Attachment #769720 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 31

4 years ago
(In reply to alexander :surkov from comment #30)
> if (MAC)
> 
> should work here (defined in common.js)
> 
Ah, yes - thanks, I missed that.
  
> > +      SimpleTest.expectAssertions(0, 1);
> 
> isn't it permanent failure, shouldn't you have expectAssertions(1, 1)?

Yes, you're right - will change that to 'expectAssertions(1)'
(Assignee)

Comment 32

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/a9d6d86a8090
http://hg.mozilla.org/integration/mozilla-inbound/rev/08db0325dc30
https://hg.mozilla.org/mozilla-central/rev/08db0325dc30
https://hg.mozilla.org/mozilla-central/rev/a9d6d86a8090
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.