Closed Bug 1445912 Opened 2 years ago Closed 2 years ago

Remove PopupBoxObject::enableKeyboardNavigator/enableRollup/setConsumeRollupEvent in favor of DOM attributes

Categories

(Core :: XUL, task)

task
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: dao, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

The consumeoutsideclicks should be used instead.
Keywords: good-first-bug
Blocks: 1446961
(In reply to Manish Kumar from comment #1)
> :dao
> 
> need to remove these two lines only?

No. Here are the full lists of files where code needs to be removed:

https://searchfox.org/mozilla-central/search?q=enableRollup

https://searchfox.org/mozilla-central/search?q=setConsumeRollupEvent
Flags: needinfo?(dao+bmo)
Assignee: nobody → 1991manish.kumar
Assignee: 1991manish.kumar → nobody
(In reply to Dão Gottwald [::dao] from comment #0)
> The consumeoutsideclicks should be used instead.

And the noautohide attribute, respectively...
Assignee: nobody → dao+bmo
Blocks: war-on-xbl
Status: NEW → ASSIGNED
Summary: Remove unused PopupBoxObject::enableRollup and PopupBoxObject::setConsumeRollupEvent → Remove PopupBoxObject::enableKeyboardNavigator/enableRollup/setConsumeRollupEvent in favor of DOM attributes
Comment on attachment 8967300 [details] [diff] [review]
patch

-void
-nsXULPopupManager::EnableRollup(nsIContent* aPopup, bool aShouldRollup)
-{
-#ifndef MOZ_GTK
-  nsMenuChainItem* item = mPopups;
-  while (item) {

The caller to EnableRollup was incorrectly removed by bug 1278282. It should be restored, or at least, don't remove this code.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8967300 - Attachment is obsolete: true
Attachment #8967300 - Flags: review?(enndeakin)
Attachment #8967348 - Flags: review?(enndeakin)
Attachment #8967348 - Flags: review?(enndeakin) → review+
Also, I think you could now remove the PopupBoxObject include and the using line near the top of nsMenuPopupFrame.cpp since the popupboxobbject isn't used the file any more.
(In reply to Neil Deakin from comment #7)
> Also, I think you could now remove the PopupBoxObject include and the using
> line near the top of nsMenuPopupFrame.cpp since the popupboxobbject isn't
> used the file any more.

This came almost too late as I was already trying to push this, but without luck:

remote: WebIDL file dom/webidl/PopupBoxObject.webidl altered in changeset 78dab4ac5ae4 without DOM peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed

*sigh*

Obviously this isn't a Web API.
Attached patch patch v3Splinter Review
Attachment #8967348 - Attachment is obsolete: true
Attachment #8967363 - Flags: review?(enndeakin)
Attachment #8967363 - Flags: review?(bzbarsky)
Attachment #8967363 - Flags: review?(enndeakin) → review+
Attachment #8967363 - Flags: review?(bzbarsky) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c5124a18c41
Remove PopupBoxObject::enableKeyboardNavigator/enableRollup/setConsumeRollupEvent in favor of DOM attributes. r=enn,peterv
https://hg.mozilla.org/mozilla-central/rev/7c5124a18c41
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(In reply to Dão Gottwald [::dao] from comment #12)
> page to remove:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/
> setConsumeRollupEvent

Page deleted; let me know if the deletion message seems appropriate. Thanks!
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.