Closed Bug 554991 Opened 11 years ago Closed 11 years ago

allow tab context menu to be modified by normal XUL overlays


(Firefox :: Tabbed Browser, defect)

Not set



Firefox 3.7a4


(Reporter: Gavin, Assigned: Gavin)



(2 files, 1 obsolete file)

... 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)
Attached patch WIP (obsolete) — Splinter Review
Assignee: nobody →
Attached patch patchSplinter Review
-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)
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
Attached patch test fixSplinter Review
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)
(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.
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 on attachment 435270 [details] [diff] [review]
test fix

See my previous comment.
Attachment #435270 - Flags: feedback?(marco.zehe) → feedback+
Closed: 11 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.