554991 - allow tab context menu to be modified by normal XUL overlays

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

... by taking it out of the tab container's anonymous content and just putting it in browser.xul directly.

(See bug 554279 comment 16 and bug 554165)

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: WIP

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

-moves the context menu to browser.xul
-moves the relevant entities to browser.dtd (leaving a copy of closeTab.label since it is also used elsewhere in tabbrowser.xml)
-removes _contextMenuAction and just calls the relevant methods directly
-moves other helper code to TabContextMenu object in browser.js
-updates tabbrowser's mContextTab property accordingly

The tab container's contextMenu property will fail if someone changes it to context="_child", but I think that's OK...

Comment 3

[:Gavin Sharp [email: [e-mail]]]

This patch is causing test_tabbrowser.xul to fail, and I'm not sure why offhand:

13011 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/tree/test_tabbrowser.xul | Different amount of expected children of [ 'tabbrowser-tabs' , role: pagetablist]. - got 4, expected 5

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: test fix

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 435270 [details] [diff] [review]
test fix

This is okay. Screen readers don't report the presence of context menu potential.

Just in case, Marco can you apply this patch and throw it at NVDA to make sure they aren't hard coding any assumptions about the children of page tab lists? Since this is unlikely I think the patch can land before Marco's results. (It would become NVDA's bug anyways).

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
>. (It would become NVDA's bug anyways).

but that's not point to break NVDA ;)

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #6)
> (In reply to comment #5)
> >. (It would become NVDA's bug anyways).
> 
> but that's not point to break NVDA ;)

Right. We want minimal time (ideally zero) for there to be breakage. That's why I want Marco's feedback ;) In this particular case, if there is breakage, we will likely drive the fix in NVDA I think.

Comment 8

[Marco Zehe (:MarcoZ)]

This patch no longer applies. But from what I know of NVDA, they won't have a problem with this change. So I suggest to put in a new patch and/or kick off a try-server build, or just land it with the test fix. Should be safe.

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 435270 [details] [diff] [review]
test fix

See my previous comment.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/7f9ba4ef648d

Comment 11

[Marco Zehe (:MarcoZ)]

Just some quick feedback that I don't detect any NVDA breackage from this bug in Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.3a4pre) Gecko/20100405 Minefield/3.7a4pre (.NET CLR 3.5.30729)