Last Comment Bug 593645 - use popup.triggerNode instead of document.popupNode for tab context menu
: use popup.triggerNode instead of document.popupNode for tab context menu
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 12
Assigned To: Frank Yan (:fryn)
:
Mentors:
Depends on: 383930
Blocks: 719739
  Show dependency treegraph
 
Reported: 2010-09-04 18:06 PDT by Frank Yan (:fryn)
Modified: 2016-01-12 14:04 PST (History)
5 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 of 2 (wait for part 2 to check this in) (1.01 KB, patch)
2010-09-04 18:06 PDT, Frank Yan (:fryn)
dao+bmo: review+
joe: approval2.0-
Details | Diff | Review
part 2 of 2: fix tests (6.30 KB, patch)
2011-12-07 18:21 PST, Frank Yan (:fryn)
no flags Details | Diff | Review
part 2 of 2: fix tests v2 (6.15 KB, patch)
2012-01-17 15:27 PST, Frank Yan (:fryn)
no flags Details | Diff | Review
part 2 of 2: fix tests v2 (6.99 KB, patch)
2012-01-18 02:52 PST, Frank Yan (:fryn)
dao+bmo: review+
Details | Diff | Review

Description Frank Yan (:fryn) 2010-09-04 18:06:51 PDT
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.
Comment 1 Frank Yan (:fryn) 2010-09-08 15:30:40 PDT
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?
Comment 2 Dão Gottwald [:dao] 2010-09-09 00:11:04 PDT
That sounds like it would complicate code currently accessing TabContextMenu.contextTab without direct access to the popup node.
Comment 3 Frank Yan (:fryn) 2010-11-05 13:48:31 PDT
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.
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2011-03-27 20:44:05 PDT
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>.
Comment 5 Frank Yan (:fryn) 2011-03-28 09:59:53 PDT
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.
Comment 6 Frank Yan (:fryn) 2011-12-07 18:21:15 PST
Created attachment 579929 [details] [diff] [review]
part 2 of 2: fix tests
Comment 7 Frank Yan (:fryn) 2012-01-03 21:32:41 PST
Comment on attachment 579929 [details] [diff] [review]
part 2 of 2: fix tests

oops, forgot to fill the requestee field.
Comment 8 Dão Gottwald [:dao] 2012-01-05 03:27:25 PST
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?
Comment 9 Frank Yan (:fryn) 2012-01-17 15:27:13 PST
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.
Comment 10 Frank Yan (:fryn) 2012-01-18 02:52:39 PST
Created attachment 589448 [details] [diff] [review]
part 2 of 2: fix tests v2

Added line to Makefile.in that I forgot.
Comment 11 Dão Gottwald [:dao] 2012-01-18 03:20:00 PST
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.
Comment 13 Frank Yan (:fryn) 2012-01-18 10:28:45 PST
Pushed with review comment addressed.
Comment 15 Serge Gautherie (:sgautherie) 2012-02-01 02:11:34 PST
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?
Comment 16 Serge Gautherie (:sgautherie) 2015-12-07 01:53:58 PST
(In reply to Serge Gautherie (:sgautherie) from comment #15)

Increased to "Found 106 matching lines in 47 files".

Note You need to log in before you can comment on or make changes to this bug.