Closed
Bug 719739
Opened 12 years ago
Closed 8 years ago
Use popup.triggerNode instead of document.popupNode for tab context menu (q.v. Bug 593645).
Categories
(SeaMonkey :: Tabbed Browser, defect)
SeaMonkey
Tabbed Browser
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.46
People
(Reporter: philip.chee, Assigned: gulm2, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(2 files)
|
15.74 KB,
patch
|
philip.chee
:
review-
|
Details | Diff | Splinter Review |
|
1012 bytes,
patch
|
philip.chee
:
review+
|
Details | Diff | Splinter Review |
From Bug 593645 Comment 0: > document.popupNode is now deprecated. Let's use the new popup.triggerNode instead.
|
||
Comment 1•12 years ago
|
||
http://mxr.mozilla.org/comm-central/search?string=.popupNode&case=1&find=%2Fsuite%2F "Found 40 matching lines in 14 files" Should they all be fixed (see my bug 593645 comment 15 questions), or just a few of them (as bug 593645 currently did)? http://mxr.mozilla.org/comm-central/search?string=this.mContextTab+%3D&case=1&find=%2Fsuite%2F "Found 2 matching lines in 2 files" tabbrowser.xml has a aPopupMenu parameter: bug 593645 should apply fine. tabmail.xml has not: should anything be done there? What? SeaMonkey has not ported the Firefox tests currently modified by bug 593645 and no other test uses this: nothing to do.
|
Reporter | |
Updated•12 years ago
|
Whiteboard: [mentor=Neil][lang=js] → [good first bug][mentor=Neil][lang=js]
|
||
Updated•12 years ago
|
Assignee: nobody → marioalv.mozilla
|
|
||
Updated•12 years ago
|
Assignee: marioalv.mozilla → nobody
|
||
Updated•10 years ago
|
Mentor: neil.corlett
Whiteboard: [good first bug][mentor=Neil][lang=js] → [good first bug][lang=js]
|
|
||
Comment 4•9 years ago
|
||
(In reply to gulm2 from comment #2) > Created attachment 8696274 [details] [diff] [review] > 18688.patch Please, add a link to a (successful) Try run. (In reply to gulm2 from comment #3) > Can this bug be assigned to me? Done.
Assignee: nobody → gulm2
Status: NEW → ASSIGNED
Attachment #8696274 -
Flags: review?(philip.chee)
|
|
Reporter | |
Comment 5•9 years ago
|
||
Comment on attachment 8696274 [details] [diff] [review] 18688.patch This bug specifically for fixing the tab context menu so this is the only section you need to change: > diff -r 43a6994f3a8b -r 17c69bb6d970 suite/browser/tabbrowser.xml > --- a/suite/browser/tabbrowser.xml Sat Dec 05 18:10:26 2015 +0000 > +++ b/suite/browser/tabbrowser.xml Sat Dec 05 19:54:36 2015 +0000 > @@ -948,7 +948,7 @@ > <parameter name="aPopupMenu"/> > <body> > <![CDATA[ > - this.mContextTab = document.popupNode; > + this.mContextTab = popup.triggerNode; > // The user might right-click on a tab or an empty part of the tabbar. > var isTab = this.mContextTab.localName == "tab"; > var isMultiple = this.tabs.length > 1; "popup" isn't a global like "document". In the developer documentation "popup" is used as a placeholder for the <menupopup> element which has the onpopupshowing event listener. > <method name="updatePopupMenu"> > <parameter name="aPopupMenu"/> > <body> > <![CDATA[ > this.mContextTab = document.popupNode; In this case the relevant menupopup is the "aPopupMenu" parameter. You should change: this.mContextTab = document.popupNode to this.mContextTab = aPopupMenu.triggerNode Please submit a new patch. Don't forget to use a more informative commit message. Thank you. If you want to fix all the other uses of document.popupNode, please file a new bug to that effect.
Flags: needinfo?(neil.corlett) → needinfo?(gulm2)
Attachment #8696274 -
Flags: review?(philip.chee) → review-
Flags: needinfo?(gulm2)
Attachment #8697431 -
Flags: review?(philip.chee)
Comment on attachment 8697431 [details] [diff] [review] New patch, fixed issues in previous patch Review of attachment 8697431 [details] [diff] [review]: ----------------------------------------------------------------- Please review
|
|
Reporter | |
Comment 8•8 years ago
|
||
Comment on attachment 8697431 [details] [diff] [review] New patch, fixed issues in previous patch Very sorry for the extremely late review. r=me I'll check this in for you. http://hg.mozilla.org/comm-central/rev/9a6d7706d5ca
Attachment #8697431 -
Flags: review?(philip.chee) → review+
|
|
Reporter | |
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.46
You need to log in
before you can comment on or make changes to this bug.
Description
•