Closed
Bug 326090
Opened 19 years ago
Closed 19 years ago
<hr> not being exposed as ROLE_SEPARATOR
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: aaronlev, Assigned: gaomingcn)
References
Details
(Keywords: access, fixed1.8.1)
Attachments
(3 files, 1 obsolete file)
12 bytes,
text/html
|
Details | |
2.98 KB,
patch
|
aaronlev
:
review-
|
Details | Diff | Splinter Review |
2.77 KB,
patch
|
aaronlev
:
review+
roc
:
superreview+
aaronlev
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
When an <hr> occurs we are supposed to expose ROLE_SEPARATOR.
Here is the class used to support that (nsHTMLHRAccessible):
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLTextAccessible.cpp#116
It's probably not being created at all.
Reporter | ||
Comment 1•19 years ago
|
||
Reporter | ||
Comment 2•19 years ago
|
||
This is a good bug to learn the accessibility architecture from.
CreateHTMLHRAccessible() is only called at:
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsInlineFrame.cpp#828
807 #ifdef ACCESSIBILITY
808 NS_IMETHODIMP nsInlineFrame::GetAccessible(nsIAccessible** aAccessible)
809 {
....
827 // Create accessible for <hr>
828 return accService->CreateHTMLHRAccessible(NS_STATIC_CAST(nsIFrame*, this), aAccessible);
829 }
830
831 return NS_ERROR_FAILURE;
832 }
833 #endif
so nsInlineFrame might not be used for <hr>. I am checking the code to find where the nsInlineFrame should be used for <hr>.
I have built firefox on suse linux, but met "cannot open display" error when I run firefox. I saw there is a bug for that(246313). Trying to build on redhat...
https://bugzilla.mozilla.org/show_bug.cgi?id=177453 is also about <hr> to expose ROLE_SEPARATOR. It shows how it is implemented. in comment#4, it said "We'll remove it from MSAA if it causes problems in Windows." It is in 2002, not sure if it was made available for windows later.
I installed the oldest version I can find on http://archive.mozilla.org/pub/firefox/nightly/2004-02-10-08-trunk/, it is not exposed either.
Investigating...
I found a change on the implementation code at:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/generic&command=DIFF_FRAMESET&file=nsInlineFrame.cpp&rev2=3.214&rev1=3.213
The change is in the patch for bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=225837
According to the output of viewer.exe:
------------------------------------------------------------------------
line 0177B3E4: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4008] bm=120 {0,420,8940,30} ca={0,420,8940,30} <
Block(hr)(1)@0177B320 next=0177B374 {0,420,8940,30} sc=0177B274(i=0,b=0)<
>
------------------------------------------------------------------------
<hr> is considered as block.
From http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/style/html.css, we get:
------------------------------------------------------------------------
378 hyatt 3.1 hr {
379 display: block;
380 caillon 3.169 height: 2px;
381 border: 1px -moz-bg-inset;
382 margin: 0.5em auto 0.5em auto;
383 -moz-float-edge: margin-box;
384 -moz-box-sizing: border-box;
385 }
------------------------------------------------------------------------
and:
------------------------------------------------------------------------
3.169 <caillon@returnzero.com> 2003-07-30 01:12
Bug 38370.
Allow color of an HR element to be changed.
Make HR be a block element in quirks mode instead of the hacky inline we were previously using (standards mode already had it as a block).
Patch by Ian Hickson <ian@hixie.ch> with minor modifications by me.
r+sr=bzbarsky@mit.edu
------------------------------------------------------------------------
So the solution might be to move CreateHTMLHRAccessible() from nsInlineFrame::GetAccessible() to nsBlockFrame::GetAccessible().
I will set up the debug enviroment and test this.
Thanks very much for Aaron's help:)
Reporter | ||
Updated•19 years ago
|
Assignee: aaronleventhal → gaomingcn
Reporter | ||
Updated•19 years ago
|
Blocks: fox2access
I have tested this patch on FC5. It works and <hr> can be exposed as ROLE_SEPARATOR(I saw "separator" in ap-poke's window).
Attachment #219497 -
Flags: review?(aaronleventhal)
Reporter | ||
Comment 8•19 years ago
|
||
Comment on attachment 219497 [details] [diff] [review]
Move CreateHTMLHRAccessible() from nsInlineFrame::GetAccessible() to nsBlockFrame::GetAccessible()
Almost there!
Here are my very small complaints, mostly just want you to use Mozilla coding style for everything (http://www.mozilla.org/hacking/mozilla-style-guide.html)
+ // Added for bug#326090. treat <hr> as block element instead of inline.
Don't specify bug numbers here, you will say that in the checkin comments so people will be able to find the bug numbers that way.
+ nsIAtom *tagAtom = mContent->Tag();
+ if (tagAtom == nsHTMLAtoms::hr) return accService->CreateHTMLHRAccessible(NS_STATIC_CAST(nsIFrame*, this), aAccessible);
No need for tagAtom temp variable.
Use separate lines for condition and the code that follows.
Also, in this and most files they use always curly braces even if there is only 1 line to group. However, just so you know that part depends on what the module owner has decided.
+ // changed for bug#326090. treat <hr> as block element instead of inline.
+ //tagAtom == nsHTMLAtoms::label || tagAtom == nsHTMLAtoms::hr) &&
+
Don't comment out removed code, just remove it. People will find it through cvs queries (easy via bonsai.mozilla.org or lxr.mozilla.org) or when they come look at this bug because the checkin comments will point them here.
Attachment #219497 -
Flags: review?(aaronleventhal) → review-
Reporter | ||
Comment 9•19 years ago
|
||
(So now you submit a new patch with those changes and I'll mark r+)
Assignee | ||
Comment 10•19 years ago
|
||
Update of the former patch.
Attachment #219580 -
Flags: review?(aaronleventhal)
Reporter | ||
Comment 11•19 years ago
|
||
Comment on attachment 219580 [details] [diff] [review]
(updated)Move CreateHTMLHRAccessible() from nsInlineFrame::GetAccessible() to nsBlockFrame::GetAccessible()
Hmm, there seems to be a file missing this time and the comments are still not right. Don't refer to other bug #'s and just completely remove the old code (don't comment out old code).
Attachment #219580 -
Flags: review?(aaronleventhal) → review-
Assignee | ||
Comment 12•19 years ago
|
||
Attachment #219580 -
Attachment is obsolete: true
Attachment #219703 -
Flags: review?(aaronleventhal)
Reporter | ||
Updated•19 years ago
|
Attachment #219703 -
Flags: review?(aaronleventhal) → review+
Reporter | ||
Updated•19 years ago
|
Attachment #219703 -
Flags: superreview?(roc)
Attachment #219703 -
Flags: superreview?(roc) → superreview+
Reporter | ||
Updated•19 years ago
|
Attachment #219703 -
Flags: approval-branch-1.8.1+
Reporter | ||
Updated•19 years ago
|
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•19 years ago
|
Keywords: fixed1.8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•