<hr> not being exposed as ROLE_SEPARATOR

RESOLVED FIXED

Status

()

RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: aaronlev, Assigned: gaomingcn)

Tracking

({access, fixed1.8.1})

Trunk
x86
All
access, fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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

13 years ago
Created attachment 210874 [details]
Test case
(Reporter)

Comment 2

13 years ago
This is a good bug to learn the accessibility architecture from.
(Assignee)

Comment 3

13 years ago
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...
(Assignee)

Comment 4

13 years ago
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...
(Assignee)

Comment 6

13 years ago
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

13 years ago
Assignee: aaronleventhal → gaomingcn
(Reporter)

Updated

13 years ago
Blocks: 333488
(Assignee)

Comment 7

13 years ago
Created attachment 219497 [details] [diff] [review]
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).
Attachment #219497 - Flags: review?(aaronleventhal)
(Reporter)

Comment 8

13 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

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

Comment 10

13 years ago
Created attachment 219580 [details] [diff] [review]
(updated)Move CreateHTMLHRAccessible() from nsInlineFrame::GetAccessible() to nsBlockFrame::GetAccessible()

Update of the former patch.
Attachment #219580 - Flags: review?(aaronleventhal)
(Reporter)

Comment 11

13 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

13 years ago
Created attachment 219703 [details] [diff] [review]
sorry! I uploaded the wrong file which has a similar file name last time
Attachment #219580 - Attachment is obsolete: true
Attachment #219703 - Flags: review?(aaronleventhal)
(Reporter)

Updated

13 years ago
Attachment #219703 - Flags: review?(aaronleventhal) → review+
(Reporter)

Updated

13 years ago
Attachment #219703 - Flags: superreview?(roc)
Attachment #219703 - Flags: superreview?(roc) → superreview+
(Reporter)

Updated

13 years ago
Attachment #219703 - Flags: approval-branch-1.8.1+
(Reporter)

Updated

13 years ago
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Reporter)

Updated

13 years ago
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.