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

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Asaf Romano (gone)]

Attachment: example

Comment 1

[Asaf Romano (gone)]

Attachment: native ui

Comment 2

[Stefan [:stefanh]]

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

Comment 3

[Stefan [:stefanh]]

Attachment: Screenshots

Comment 4

[Asaf Romano (gone)]

Why did you repeat the box-shadow rule in preferences?

Comment 5

[Stefan [:stefanh]]

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

Comment 6

[Stefan [:stefanh]]

That said, perhaps we should also style the html links in global.css... I'll have look tomorrow.

Comment 7

[Stefan [:stefanh]]

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

Comment 8

[Stefan [:stefanh]]

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.

Comment 9

[Asaf Romano (gone)]

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.

Comment 10

[Stefan [:stefanh]]

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

Comment 11

[Stefan [:stefanh]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bfa11f026529

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Stefan [:stefanh]]

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.

Comment 14

[Stefan [:stefanh]]

Looks like it's the box-shadow rule in global.css that causes the assertion...

Comment 15

[Stefan [:stefanh]]

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.

Comment 16

[Stefan [:stefanh]]

> 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/[e-mail]-c2f7a1c4a0ad/

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

Comment 17

[alexander :surkov (:asurkov)]

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

Comment 18

[Stefan [:stefanh]]

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

Comment 19

[Stefan [:stefanh]]

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

[alexander :surkov (:asurkov)]

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

Comment 21

[Stefan [:stefanh]]

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

Comment 22

[Stefan [:stefanh]]

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?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not sure what the problem is but I bet Markus can figure it out :-)

Comment 24

[Markus Stange [: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).

Comment 25

[alexander :surkov (:asurkov)]

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

Comment 26

[Trevor Saunders (:tbsaunde)]

(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

[alexander :surkov (:asurkov)]

(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

Comment 28

[Stefan [:stefanh]]

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

Comment 29

[Stefan [:stefanh]]

Attachment: Fix test_general.xul to expect 1 assertion on mac

Sorry for the delay, but here's comment #27 implemented

Comment 30

[alexander :surkov (:asurkov)]

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)?

Comment 31

[Stefan [:stefanh]]

(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)'

Comment 32

[Stefan [:stefanh]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a9d6d86a8090
http://hg.mozilla.org/integration/mozilla-inbound/rev/08db0325dc30

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/08db0325dc30
https://hg.mozilla.org/mozilla-central/rev/a9d6d86a8090