use popup.triggerNode instead of document.popupNode for tab context menu

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: fryn, Assigned: fryn)

Tracking

Trunk
Firefox 12
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 472216 [details] [diff] [review]
part 1 of 2 (wait for part 2 to check this in)

Since bug 383930 was fixed and according to Neil Deakin's blog post (http://enndeakin.wordpress.com/2010/08/08/status-update-for-august-7/), document.popupNode is now deprecated. Let's use the new popup.triggerNode instead.
Attachment #472216 - Flags: review?(dao)

Updated

7 years ago
Attachment #472216 - Flags: review?(dao) → review+
(Assignee)

Updated

7 years ago
Attachment #472216 - Flags: approval2.0?
(Assignee)

Comment 1

7 years ago
Dao, since we now only show the tab context menu on the tabs themselves, can we also get rid of TabContextMenu.contextTab and just directly use aPopupMenu.triggerNode?
That sounds like it would complicate code currently accessing TabContextMenu.contextTab without direct access to the popup node.
Attachment #472216 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 3

7 years ago
turns out that some tests rely on being able to set document.popupNode arbitarily, so I'll have to fix the tests.

Low priority, so I'll come back to this later.
Attachment #472216 - Flags: approval2.0+ → approval2.0-
Indeed.  I landed it <http://hg.mozilla.org/projects/cedar/rev/a4a1307fc4e1>, it broke tests <http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1301271675.1301274934.12522.gz> and Karl backed it out <http://hg.mozilla.org/projects/cedar/rev/bbacb90f4c63>.
Whiteboard: not-ready
(Assignee)

Comment 5

6 years ago
Comment on attachment 472216 [details] [diff] [review]
part 1 of 2 (wait for part 2 to check this in)

(In reply to comment #4)

Yeah, I warned about that. The tests shouldn't be using document.popupNode like that, even though it's the easy way. popup.triggerNode wasn't available then, so also no need to blame 'em. I'll get to this later.
Attachment #472216 - Attachment description: patch → part 1 of 2 (wait for part 2 to check this in)
(Assignee)

Comment 6

6 years ago
Created attachment 579929 [details] [diff] [review]
part 2 of 2: fix tests
Attachment #579929 - Flags: review?
(Assignee)

Comment 7

6 years ago
Comment on attachment 579929 [details] [diff] [review]
part 2 of 2: fix tests

oops, forgot to fill the requestee field.
Attachment #579929 - Flags: review? → review?(dao)
Comment on attachment 579929 [details] [diff] [review]
part 2 of 2: fix tests

>+  let tab = gBrowser.tabs[0];
>+  let menu = document.getElementById("tabContextMenu");
>+  menu.openPopup(tab, "end_after", 0, 0, true, false, {target: tab});
>+  TabContextMenu.updateContextMenu(menu);
>+  menu.hidePopup();

Can you add a helper function for this in head.js?

Updated

5 years ago
Attachment #579929 - Flags: review?(dao)
(Assignee)

Comment 9

5 years ago
Created attachment 589322 [details] [diff] [review]
part 2 of 2: fix tests v2

(In reply to Dão Gottwald [:dao] from comment #8)
... 
> Can you add a helper function for this in head.js?

Done.
Attachment #579929 - Attachment is obsolete: true
Attachment #589322 - Flags: review?(dao)
(Assignee)

Comment 10

5 years ago
Created attachment 589448 [details] [diff] [review]
part 2 of 2: fix tests v2

Added line to Makefile.in that I forgot.
Attachment #589322 - Attachment is obsolete: true
Attachment #589448 - Flags: review?(dao)
Attachment #589322 - Flags: review?(dao)
Comment on attachment 589448 [details] [diff] [review]
part 2 of 2: fix tests v2

>+++ b/browser/base/content/test/browser_visibleTabs_contextMenu.js

> function popup(tab) {
>-  document.popupNode = tab;
>-  TabContextMenu.updateContextMenu(document.getElementById("tabContextMenu"));
>+  let menu = document.getElementById("tabContextMenu");
>+  menu.openPopup(tab, "end_after", 0, 0, true, false, {target: tab});
>+  TabContextMenu.updateContextMenu(menu);
>   is(TabContextMenu.contextTab, tab, "TabContextMenu context is the expected tab");
>-  TabContextMenu.contextTab = null;
>+  menu.hidePopup();
> }

>+++ b/browser/base/content/test/head.js

>+function updateTabContextMenu() {
>+  let tab = gBrowser.selectedTab;
>+  let menu = document.getElementById("tabContextMenu");
>+  menu.openPopup(tab, "end_after", 0, 0, true, false, {target: tab});
>+  TabContextMenu.updateContextMenu(menu);
>+  menu.hidePopup();
>+}

updateTabContextMenu can take an optional tab argument and integrate the test that popup() above is doing.
Attachment #589448 - Flags: review?(dao) → review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/224d7c74206f
https://hg.mozilla.org/integration/fx-team/rev/8be167cb61af
Whiteboard: not-ready
Target Milestone: --- → Firefox 12
(Assignee)

Comment 13

5 years ago
Pushed with review comment addressed.
https://hg.mozilla.org/mozilla-central/rev/224d7c74206f
https://hg.mozilla.org/mozilla-central/rev/8be167cb61af
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
http://mxr.mozilla.org/mozilla-central/search?string=.popupNode&case=1
"Found 72 matching lines in 30 files"

Should this bug (have) fix these too?
Should they be fixed in a separate bug(s)?
Should they not be fixed?
Blocks: 719739
Depends on: 383930
Flags: in-testsuite+
(In reply to Serge Gautherie (:sgautherie) from comment #15)

Increased to "Found 106 matching lines in 47 files".
Flags: needinfo?(fryn)
(Assignee)

Updated

a year ago
Flags: needinfo?(fryn)
You need to log in before you can comment on or make changes to this bug.