Closed Bug 491851 Opened 11 years ago Closed 11 years ago

implement relations inside HTML table

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch wip (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
Attachment #376249 - Attachment is obsolete: true
Attachment #376844 - Flags: superreview?(neil)
Attachment #376844 - Flags: review?(marco.zehe)
Attachment #376844 - Flags: review?(david.bolter)
Attached patch patch2 (obsolete) — Splinter Review
small improvement.
Attachment #376844 - Attachment is obsolete: true
Attachment #376847 - Flags: superreview?(neil)
Attachment #376847 - Flags: review?(marco.zehe)
Attachment #376847 - Flags: review?(david.bolter)
Attachment #376844 - Flags: superreview?(neil)
Attachment #376844 - Flags: review?(marco.zehe)
Attachment #376844 - Flags: review?(david.bolter)
Comment on attachment 376847 [details] [diff] [review]
patch2

>+  /**
>+   * Query nsAccessNode from the given nsIAccessNode.
>+   */
>+  static already_AddRefed<nsHTMLTableAccessible>
>+    QueryAccessibleTable(nsIAccessibleTable *aAccessibleTable);

The description for this function is completely bogus. :-) It should read something like "Query the nsIAccessibleTable interface from the given accessible."

>+nsresult
>+nsHTMLTableHeaderAccessible::GetRoleInternal(PRUint32 *aRole)
>+{
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>+
>+  // Check value of @scope attribute.
>+  if (content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::scope,
>+                           nsAccessibilityAtoms::col, eIgnoreCase)) {
>+    *aRole = nsIAccessibleRole::ROLE_COLUMNHEADER;
>+    return NS_OK;
>+  }
>+
>+  if (content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::scope,
>+                           nsAccessibilityAtoms::row, eIgnoreCase)) {

Would it perhaps be more performant if we queried that attrValue once, store it in a local variable and then simply compare that against the atoms? It strikes me as not right to do this query twice.

>+  /**
>+   *
>+   */
>+  nsresult FindCellsForRelation(PRInt32 aSearchHint, PRUint32 aRelationType,
>+                                nsIAccessibleRelation **aRelation);
>+
>+  /**
>+   *
>+   */
>+  nsIContent* FindCell(nsHTMLTableAccessible *aTableAcc, nsIContent *aAnchorCell,
>+                       PRInt32 aRowIdx, PRInt32 aColIdx,
>+                       PRInt32 aLookForHeader);
>+};

Nit: Add proper comments please.

>+    <tobdy>

Typo :-)

>+      <td id="table2_3" scope="col">col1</th>

Wrong closing tag, should be </td>. The other cell below that has it correct.
Attached patch patchSplinter Review
Attachment #376847 - Attachment is obsolete: true
Attachment #376879 - Flags: superreview?(neil)
Attachment #376879 - Flags: review?(marco.zehe)
Attachment #376879 - Flags: review?(david.bolter)
Attachment #376847 - Flags: superreview?(neil)
Attachment #376847 - Flags: review?(marco.zehe)
Attachment #376847 - Flags: review?(david.bolter)
Comment on attachment 376879 [details] [diff] [review]
patch

The tests look great. I also don't see anything else in the patch that sticks out at me, but leaving that to davidb and neil for r and sr. r=me.
Attachment #376879 - Flags: review?(marco.zehe) → review+
A drive by concern is around performance. Walking dom for each TD is a bit scary no? I guess this work will help treegrid support?
(In reply to comment #7)
> A drive by concern is around performance. Walking dom for each TD is a bit
> scary no? I guess this work will help treegrid support?

David, thinking more I think it's not scary because we do more ugly walking through DOMs when we calculate description_for relations from aria-describedby. So it shouldn't hit us. Since this algorithm is proposed by HTML 4.1 spec then it's worth to follow it until we meet performance problem.
Attachment #376879 - Flags: superreview?(neil) → superreview+
Blocks: 287740
Attachment #376879 - Flags: review?(david.bolter) → review+
Is our table/grid/etc work going to be landed on 1.9.1?
(In reply to comment #10)
> Is our table/grid/etc work going to be landed on 1.9.1?

As far as I understood Rich Schwerdtfeger, no, it suffices for Firefox 3.next.
http://hg.mozilla.org/mozilla-central/rev/2322a5800d59
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 492961
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090517 Minefield/3.6a1pre (.NET CLR 3.5.30729)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.