Closed Bug 1517536 Opened 6 years ago Closed 6 years ago

[de-xbl] Migrate popup-menu to custom element.

Categories

(Thunderbird :: Mail Window Front End, task)

Product:
Thunderbird
Component:
Mail Window Front End
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 66.0

People

(Reporter: arshad, Assigned: arshad)

References

Details

Attachments

(1 file, 5 obsolete files)

popup-menu.patch
19.15 KB, patch
Details | Diff | Splinter Review
It appears to work but there is some issue with the animation of facet-discrete elment. I am adding the facet-discrete binding to dependent field and will fix the animation once facet-discrete custom element lands.
Attached patch popup-menu.patch (obsolete) — — Splinter Review
Assignee: nobody → arshdkhn1
Blocks: tb-war-on-xbl
Depends on: 1516647
Comment on attachment 9034220 [details] [diff] [review] popup-menu.patch Review of attachment 9034220 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/glodaFacet.js @@ +67,5 @@ > + super(); > + > + this.addEventListener("keypress", (event) => { if (event.keyCode != KeyEvent.DOM_VK_ESCAPE) { return; } this.hide(); }); > + > + this.addEventListener("keypress", (event) => { if (event.keyCode != KeyEvent.DOM_VK_DOWN) { return; } this.moveFocus(event, 1); }); please just use one event listener for all of thes, then switch on the event.keyCode
Attached patch popup-menu.patch (obsolete) — — Splinter Review
Attachment #9034220 - Attachment is obsolete: true
Attached patch popup-menu.patch (obsolete) — — Splinter Review
Attachment #9037260 - Attachment is obsolete: true
Attached patch popup-menu.patch (obsolete) — — Splinter Review
Attachment #9037266 - Attachment is obsolete: true
Comment on attachment 9037550 [details] [diff] [review] popup-menu.patch Review of attachment 9037550 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/glodaFacet.js @@ +973,5 @@ > + this.moveFocus(event, 1); > + break; > + > + case KeyEvent.DOM_VK_TAB: > + if (event.shiftKey) { if shift key is also pressed then this if condition will be executed.. also even when opt/fn key is pressed along with shift and VK_TAB this condition will be executed.. For old xbl popup-menu fn+shift+tab also executed this condition but with opt+shift+tab this conditon will not execute.. IMHO we dont need this shift case at all, since the max no of items in popup is just two so the moveFocus(event, 1) is same as moveFocus(event, -1). ::: mail/base/content/glodaFacetView.xhtml @@ -41,5 @@ > <script type="application/javascript" > src="chrome://messenger/content/glodaFacetVis.js"></script> > </head> > <body id="body" onload="reachOutAndTouchFrame()" > - onkeypress="if (event.keyCode == event.DOM_VK_ESCAPE) document.getElementById('popup-menu').hide();" there was already a handler for this in popup-menu binding,don't know why it was added here.
Attachment #9037550 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9037550 [details] [diff] [review] popup-menu.patch Review of attachment 9037550 [details] [diff] [review]: ----------------------------------------------------------------- Seems ok to me. r=mkmelin with the comments addressed. Please also replace all the "var" with "let" ::: mail/base/content/glodaFacet.js @@ +973,5 @@ > + this.moveFocus(event, 1); > + break; > + > + case KeyEvent.DOM_VK_TAB: > + if (event.shiftKey) { For clarity, lets still keep the 1 an -1 cases. You never know when someone wants to extend the menu @@ +1111,5 @@ > + or if we're on a preselected facet value, whether included or > + excluded. The variety attribute handles that through CSS */ > + this.setAttribute("variety", variety); > + let rect = barNode.getBoundingClientRect(); > + let X, Y; while you're here, can you make these lower case x, y @@ +1171,5 @@ > customElements.define("facet-date", MozFacetDate); > customElements.define("facet-boolean", MozFacetBoolean); > customElements.define("facet-boolean-filtered", MozFacetBooleanFiltered); > customElements.define("facet-discrete", MozFacetDiscrete); > +customElements.define("popup-menu", MozPopupMenu); I think the name is too generic. Can you rename it to facet-popup-menu, and the class to MozFacetPopupMenu?
Attachment #9037550 - Flags: review?(mkmelin+mozilla) → review+
Attached patch popup-menu.patch — — Splinter Review

changing one var to let

Attachment #9037968 - Attachment is obsolete: true
Keywords: checkin-needed

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/a0a854d99ee0
Migrate popup-menu to custom element; r=mkmelin

Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 66.0
Type: defect → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: