Closed
Bug 556004
Opened 15 years ago
Closed 14 years ago
###!!! ASSERTION: Getting the link state of an unregistered Link!: 'mRegistered'
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: neil, Assigned: surkov)
References
()
Details
(Keywords: assertion)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
davidb
:
review+
MarcoZ
:
review+
davidb
:
approval2.0+
|
Details | Diff | Splinter Review |
Steps to reproduce problem:
1. Browse to any reasonable web page
2. Inspect the page in DOM Inspector, and show accessible nodes
3. Tab through all the links
On Google UK I always get it on the Shopping and Sign In links.
Comment 1•15 years ago
|
||
(In reply to comment #0)
> Steps to reproduce problem:
> 1. Browse to any reasonable web page
> 2. Inspect the page in DOM Inspector, and show accessible nodes
> 3. Tab through all the links
Does this require accessible nodes being on? My bet is that some a11y code is calling Link::GetLinkState when it really wants to be calling IntrinsicState and checking the link bits.
Reporter | ||
Updated•15 years ago
|
Component: Places → Disability Access APIs
Product: Toolkit → Core
QA Contact: places → accessibility-apis
Comment 2•15 years ago
|
||
(In reply to comment #1)
> (In reply to comment #0)
> > Steps to reproduce problem:
> > 1. Browse to any reasonable web page
> > 2. Inspect the page in DOM Inspector, and show accessible nodes
> > 3. Tab through all the links
> Does this require accessible nodes being on?
Yeah.
> My bet is that some a11y code is
> calling Link::GetLinkState when it really wants to be calling IntrinsicState
> and checking the link bits.
We do call GetLinkState... is that naughty?
Updated•15 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•15 years ago
|
||
Just tried this really quick... we get one failure in states/test_link.html. Need to look into this (IntrinsicState) a bit more (later).
Assignee | ||
Comment 5•15 years ago
|
||
Description from dupe bug 560113:
The assertion appears whenever nsIContent::GetLinkState() is called on link
that has 'eLinkState_NotLink' state
(http://mxr.mozilla.org/mozilla-central/source/content/base/src/Link.cpp#72).
It happens because mRegistered member is false when the link is not a link
(http://mxr.mozilla.org/mozilla-central/source/content/base/src/Link.cpp#122).
We get lot of assertion when accessibility is turned on. Stack trace is
> xul.dll!mozilla::dom::Link::GetLinkState() Line 72 + 0x23 bytes C++
xul.dll!nsHTMLAnchorElement::GetLinkState() Line 423 C++
xul.dll!nsHTMLLinkAccessible::GetStateInternal(unsigned int *
aState=0x001abbc4, unsigned int * aExtraState=0x00000000) Line 85 + 0x1b bytes
C++
xul.dll!nsAccessible::GetState(unsigned int * aState=0x001abbc4, unsigned
int * aExtraState=0x00000000) Line 1677 + 0x1c bytes C++
xul.dll!nsAccessibleWrap::get_accState(tagVARIANT varChild=I4 = 0,
tagVARIANT * pvarState=I4 = 0) Line 544 + 0x1f bytes C++
I suppose the fix could be
NS_ASSERTION(mRegistered || mLinkState != eLinkState_NotLink,
"Getting the link state of an unregistered Link!");
However a11y expects eLinkState_Unknown as possible value to be returned.
Assignee | ||
Comment 6•15 years ago
|
||
(In reply to comment #3)
> Created an attachment (id=436173) [details]
> quick attempt
>
> Just tried this really quick... we get one failure in states/test_link.html.
> Need to look into this (IntrinsicState) a bit more (later).
I believe it's not a11y fault and it should be fixed in link.cpp.
Comment 7•15 years ago
|
||
> I believe it's not a11y fault and it should be fixed in link.cpp.
Yeah, indeed. Unless we remove the nsIContent GetLinkState api, of course.
Comment 8•15 years ago
|
||
Or document that you have to call IsLink() first.
Assignee | ||
Comment 9•15 years ago
|
||
(In reply to comment #8)
> Or document that you have to call IsLink() first.
Either way works I think. However documentation approach doesn't sound very friendly since I would say method shouldn't assert just because it's not supposed to be called on this object.
Btw, nsIContent::IsLink() is fine but it confuses me since it has required nsIURI out argument, a11y doesn't need it and therefore it makes me think it requires unnecessary computations/constructions of uri.
So I think it's ok to call IsLink() in a11y before we call GetLinkState() but I really would like to see out argument optional if that's possible.
Comment 10•15 years ago
|
||
(In reply to comment #7)
> Yeah, indeed. Unless we remove the nsIContent GetLinkState api, of course.
Wonder why we never did that. We should probably do that...
(In reply to comment #9)
> Btw, nsIContent::IsLink() is fine but it confuses me since it has required
> nsIURI out argument, a11y doesn't need it and therefore it makes me think it
> requires unnecessary computations/constructions of uri.
Except that everything figures out if it is a link by generating the URI, so it's no additional work really.
Assignee | ||
Comment 11•14 years ago
|
||
Use IntristicState() as it was suggested and was in previous patch, enable actions/test_link that was never enabled by accident.
Assignee: nobody → surkov.alexander
Attachment #436173 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #484619 -
Flags: review?(marco.zehe)
Attachment #484619 -
Flags: review?(bolterbugz)
Updated•14 years ago
|
Attachment #484619 -
Flags: review?(marco.zehe) → review+
Comment 12•14 years ago
|
||
Comment on attachment 484619 [details] [diff] [review]
patch
r=me, glad you got to this one. So calling IntrinsicState() doesn't cause the assertion? (The patch is good clean up either way)
Attachment #484619 -
Flags: review?(bolterbugz) → review+
Assignee | ||
Comment 13•14 years ago
|
||
(In reply to comment #12)
> Comment on attachment 484619 [details] [diff] [review]
> patch
>
> r=me, glad you got to this one.
I've tired to see these in console, too much when you debug real pages.
>So calling IntrinsicState() doesn't cause the
> assertion? (The patch is good clean up either way)
no, it doesn't use GetLinkState, and I don't recall them in console :)
Assignee | ||
Updated•14 years ago
|
Attachment #484619 -
Flags: approval2.0?
Comment 14•14 years ago
|
||
Comment on attachment 484619 [details] [diff] [review]
patch
Approved. Let's get a clean try run first.
Attachment #484619 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 15•14 years ago
|
||
(In reply to comment #14)
> Comment on attachment 484619 [details] [diff] [review]
> patch
>
> Approved. Let's get a clean try run first.
I started try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-bd4dcfc7f446
I'll wait for linux results since I tested on windows locally.
Assignee | ||
Comment 16•14 years ago
|
||
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/b003e775737b
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•