Add basic ATK roles for MathML elements

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

(Blocks 1 bug)

Trunk
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [parity-webkit])

Attachments

(1 attachment, 6 obsolete attachments)

Assignee

Description

5 years ago
Bug 1001634 applies ATK_ROLE_UNKNOWN to all MathML elements. Until we add proper (sub)roles into ATK and AT-SPI, we can already change ATK_ROLE_UNKNOWN to something better. Here is what we discussed with Joanie:

* <mtable> - ATK_ROLE_TABLE (permanently)
* <mtr> - ATK_ROLE_TABLE_ROW (permanently)
* <mtd> - ATK_ROLE_TABLE_CELL (permanently)
* <mlabeledtr> - ATK_ROLE_TABLE_ROW (permanently)
* <mglyph> - ATK_ROLE_IMAGE (permanently)
* group-related elements (<mrow> etc): ATK_ROLE_PANEL (temporary)
* text-related: ATK_ROLE_STATIC (temporary)

ATK_ROLE_STATIC is currently only available in the unstable version of ATK, so we'll need to update the one used in Gecko.
Assignee

Comment 1

5 years ago
Posted patch Patch (obsolete) — Splinter Review
Assignee

Comment 2

5 years ago
Posted patch Patch (obsolete) — Splinter Review
Linux builds will be available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/fred.wang@free.fr-d6db7b46c550
Attachment #8533812 - Attachment is obsolete: true
Assignee

Comment 4

5 years ago
Posted patch Patch V2 (obsolete) — Splinter Review
This new version adds the new ATK_ROLE_STATIC and uses it for token MathML element. We're still not sure about whether ATK_ROLE_PANEL or ATK_ROLE_SECTION should be used for MathML grouping elements, so I'm just asking for feedback for now.

(this applies on top of Jonathan's patches)
Attachment #8533832 - Attachment is obsolete: true
Attachment #8534293 - Attachment is obsolete: true
Attachment #8534331 - Flags: feedback?(surkov.alexander)
Comment on attachment 8534331 [details] [diff] [review]
Patch V2

>   if (aAtkObj->role == ATK_ROLE_LIST_BOX && !IsAtkVersionAtLeast(2, 1))
>     aAtkObj->role = ATK_ROLE_LIST;
>   else if (aAtkObj->role == ATK_ROLE_TABLE_ROW && !IsAtkVersionAtLeast(2, 1))
>     aAtkObj->role = ATK_ROLE_LIST_ITEM;
> 
>+  if (aAtkObj->role == ATK_ROLE_STATIC && !IsAtkVersionAtLeast(2, 16))
>+    aAtkObj->role = ATK_ROLE_TEXT;

else if

> ROLE(MATHML_MATH,
>      "math",
>-     ATK_ROLE_UNKNOWN,
>+     ATK_ROLE_MATH,

you need a runtime check that role is available.

> ROLE(MATHML_SPACE,
>      "mathml space",
>      ATK_ROLE_UNKNOWN,

I think there's an argument to be made for this being a text role though really a accessible just for whitespace is kind of strange.
Assignee

Comment 6

5 years ago
Posted patch Patch V3 (obsolete) — Splinter Review
Thanks for the feedback Trevor. Indeed, I had forgotten to remove mspace in bug 1001634.
Attachment #8534331 - Attachment is obsolete: true
Attachment #8534331 - Flags: feedback?(surkov.alexander)
Attachment #8534902 - Flags: feedback?(surkov.alexander)
Comment on attachment 8534902 [details] [diff] [review]
Patch V3

Review of attachment 8534902 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/base/RoleMap.h
@@ +1121,5 @@
>       eNoNameRule)
>  
>  ROLE(MATHML_FRACTION,
>       "mathml fraction",
>       ATK_ROLE_UNKNOWN,

do we wait for new role for this?
Attachment #8534902 - Flags: feedback?(surkov.alexander) → review+
Assignee

Comment 8

5 years ago
(In reply to alexander :surkov from comment #7)
> do we wait for new role for this?

The idea was just to use the ATK roles that already exist, like in the corresponding WebKit bug I'm adding to the "see also" list. We started to talk about more (sub)roles with Joanie but that will be done in follow-up bugs.
Whiteboard: [parity-webkit]
Assignee

Updated

5 years ago
Blocks: 1128143
Assignee

Updated

5 years ago
No longer depends on: 1001634
Assignee

Updated

5 years ago
Depends on: 1001641
Assignee

Comment 9

5 years ago
Posted patch Patch final version (obsolete) — Splinter Review
Unbitrot
Attachment #8534902 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Depends on: 1001637
No longer depends on: 1001641
Assignee

Comment 10

4 years ago
Unbitrot and make it apply before bug 1001641.
Attachment #8557469 - Attachment is obsolete: true
Assignee

Comment 11

4 years ago
So the ATK patches can be taken before Jonathan's other patches.
Depends on: 1001634
No longer depends on: 1001637
https://hg.mozilla.org/mozilla-central/rev/fb6c18e47df5
Assignee: nobody → fred.wang
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.