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)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 66.0
People
(Reporter: arshad, Assigned: arshad)
References
Details
Attachments
(1 file, 5 obsolete files)
|
(deleted),
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.
|
Assignee | |
Comment 1•6 years ago
|
||
Assignee: nobody → arshdkhn1
|
|
Assignee | |
Updated•6 years ago
|
Blocks: tb-war-on-xbl
Depends on: 1516647
|
||
Comment 2•6 years ago
|
||
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
|
|
Assignee | |
Comment 3•6 years ago
|
||
Attachment #9034220 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 4•6 years ago
|
||
Attachment #9037260 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 5•6 years ago
|
||
Attachment #9037266 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 6•6 years ago
|
||
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 7•6 years ago
|
||
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+
|
|
Assignee | |
Comment 8•6 years ago
|
||
Attachment #9037550 -
Attachment is obsolete: true
Attachment #9037968 -
Flags: review+
|
|
Assignee | |
Comment 9•6 years ago
|
||
changing one var to let
Attachment #9037968 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•6 years ago
|
Keywords: checkin-needed
|
||
Comment 10•6 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/a0a854d99ee0
Migrate popup-menu to custom element; r=mkmelin
|
||
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 66.0
|
||
Updated•5 years ago
|
Type: defect → task
You need to log in
before you can comment on or make changes to this bug.
Description
•