326090 - <hr> not being exposed as ROLE_SEPARATOR

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

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.

Comment 1

[Aaron Leventhal]

Attachment: Test case

Comment 2

[Aaron Leventhal]

This is a good bug to learn the accessibility architecture from.

Comment 3

[Mike Gao]

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

Comment 4

[Mike Gao]

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

Comment 5

[Mike Gao]

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

Comment 6

[Mike Gao]

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 <[e-mail]> 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 <[e-mail]> with minor modifications by me.
r+sr=[e-mail]
------------------------------------------------------------------------

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

Comment 7

[Mike Gao]

Attachment: Move CreateHTMLHRAccessible() from nsInlineFrame::GetAccessible() to nsBlockFrame::GetAccessible()

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

Comment 8

[Aaron Leventhal]

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.

Comment 9

[Aaron Leventhal]

(So now you submit a new patch with those changes and I'll mark r+)

Comment 10

[Mike Gao]

Attachment: (updated)Move CreateHTMLHRAccessible() from nsInlineFrame::GetAccessible() to nsBlockFrame::GetAccessible()

Update of the former patch.

Comment 11

[Aaron Leventhal]

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

Comment 12

[Mike Gao]

Attachment: sorry! I uploaded the wrong file which has a similar file name last time