Closed Bug 326090 Opened 15 years ago Closed 15 years ago

<hr> not being exposed as ROLE_SEPARATOR

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: gaomingcn)

References

Details

(Keywords: access, fixed1.8.1)

Attachments

(3 files, 1 obsolete file)

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.
Attached file Test case
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...
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:)
Assignee: aaronleventhal → gaomingcn
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)
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-
(So now you submit a new patch with those changes and I'll mark r+)
Update of the former patch.
Attachment #219580 - Flags: review?(aaronleventhal)
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-
Attachment #219580 - Attachment is obsolete: true
Attachment #219703 - Flags: review?(aaronleventhal)
Attachment #219703 - Flags: review?(aaronleventhal) → review+
Attachment #219703 - Flags: superreview?(roc)
Attachment #219703 - Flags: superreview?(roc) → superreview+
Attachment #219703 - Flags: approval-branch-1.8.1+
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.