allow tab context menu to be modified by normal XUL overlays

RESOLVED FIXED in Firefox 3.7a4

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

unspecified
Firefox 3.7a4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

... 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)
Created attachment 434910 [details] [diff] [review]
WIP
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Created attachment 435014 [details] [diff] [review]
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...
Attachment #434910 - Attachment is obsolete: true
Attachment #435014 - Flags: review?(dao)

Updated

7 years ago
Attachment #435014 - Flags: review?(dao) → review+
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
Created attachment 435270 [details] [diff] [review]
test fix
Attachment #435270 - Flags: review?(bolterbugz)
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).
Attachment #435270 - Flags: review?(bolterbugz)
Attachment #435270 - Flags: review+
Attachment #435270 - Flags: feedback?(marco.zehe)

Comment 6

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

but that's not point to break NVDA ;)
(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

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

7 years ago
Comment on attachment 435270 [details] [diff] [review]
test fix

See my previous comment.
Attachment #435270 - Flags: feedback?(marco.zehe) → feedback+
https://hg.mozilla.org/mozilla-central/rev/7f9ba4ef648d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a4
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)
You need to log in before you can comment on or make changes to this bug.