Closed Bug 1531282 Opened 6 years ago Closed 6 years ago

MozTree event handlers should be attached in connectedCallback() instaed of constructor

Categories

(Toolkit :: UI Widgets, defect, P3)

Product:
Toolkit
Component:
UI Widgets
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: mkmelin, Assigned: mkmelin)

References

Details

Attachments

(1 file, 1 obsolete file)

bug1531282_tree_event_handler.patch
17.45 KB, patch
bgrins
: review+
Details | Diff | Splinter Review

+++ This bug was initially created as a clone of Bug #1523957 +++

As noted when investigating bug 1529872, when the event handlers are set up in the constructor, e.g. for <tree onkeydown="foo(event);"> foo() will get its turn to run in a different order than it used to, and it can't be used to cancel further event propagation (like it used to).

Adding them on connectedCallback also seems preferable in general.

Do a patch before splinter review is disabled today ;-) - Otherwise: https://phabricator.services.mozilla.com/differential/diff/create/ unless you have a Phabricator client set up.

(In reply to Magnus Melin [:mkmelin] from comment #2)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42e6948cedbdab471c22f4b4a28f4726ee6900c6

I'm fine with moving handlers to connectedCallback, but we should guard against multiple connections being fired (i.e. store a bit to prevent attaching them a second time). Perhaps make a new function setupEventListeners and then in connectedCallback do something like if (!this._eventListenersSetup) { this._eventListenersSetup = true; this.setupEventListeners() }.

Also, this error looks suspicious: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42e6948cedbdab471c22f4b4a28f4726ee6900c6&selectedJob=230982697. Could you check if that's failing locally?

Yeah that was a real failure (I had forgot the breaks when moving to switch, surprised not more things broke!)

Attachment #9047327 - Attachment is obsolete: true
Attachment #9047498 - Flags: review?(bgrinstead)
Comment on attachment 9047498 [details] [diff] [review] bug1531282_tree_event_handler.patch Review of attachment 9047498 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine, thanks. Please update the commit message to be r=bgrins ::: toolkit/content/widgets/tree.js @@ +702,5 @@ > } > }); > > + this.addEventListener("blur", (event) => { > + this.focused = false; I don't know enough about the history of the XBL binding to know if there was a reason that the `focused` property was getting set in a bubbling listener. Just looking at https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/dom/xul/XULTreeElement.cpp#163 I guess it's OK to change when this happens.
Attachment #9047498 - Flags: review?(bgrinstead) → review+
Pushed by mkmelin@iki.fi: https://hg.mozilla.org/integration/mozilla-inbound/rev/465d08059933 MozTree event handlers should be attached in connectedCallback() instead of constructor. r=bgrins

Backed out changeset 465d08059933 (Bug 1531282) for ES failure in /builds/worker/checkouts/gecko/toolkit/content/widgets/tree.js:866:5 CLOSED TREE

Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=231080794

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231080794&repo=mozilla-inbound&lineNumber=274

[vcs 2019-02-28T20:15:10.662Z]
[vcs 2019-02-28T20:15:10.662Z] 271226 files updated, 0 files merged, 0 files removed, 0 files unresolved
[vcs 2019-02-28T20:15:10.923Z] updated to 465d08059933ab8710a69843ac19a672124c7d47
[vcs 2019-02-28T20:15:10.927Z] PERFHERDER_DATA: {"framework": {"name": "vcs"}, "suites": [{"extraOptions": ["m3.xlarge"], "lowerIsBetter": true, "name": "clone", "shouldAlert": false, "subtests": [], "value": 155.15637803077698}, {"extraOptions": ["m3.xlarge"], "lowerIsBetter": true, "name": "update", "shouldAlert": false, "subtests": [], "value": 93.34527015686035}, {"extraOptions": ["m3.xlarge"], "lowerIsBetter": true, "name": "overall", "shouldAlert": false, "subtests": [], "value": 249.14377808570862}, {"extraOptions": ["m3.xlarge"], "lowerIsBetter": true, "name": "overall_clone", "shouldAlert": false, "subtests": [], "value": 249.14377808570862}, {"extraOptions": ["m3.xlarge"], "lowerIsBetter": true, "name": "overall_clone_fullcheckout", "shouldAlert": false, "subtests": [], "value": 249.14377808570862}]}
[vcs 2019-02-28T20:15:11.182Z] TinderboxPrint:<a href=https://hg.mozilla.org/integration/mozilla-inbound/rev/465d08059933ab8710a69843ac19a672124c7d47 title='Built from mozilla-inbound revision 465d08059933ab8710a69843ac19a672124c7d47'>465d08059933ab8710a69843ac19a672124c7d47</a>
[task 2019-02-28T20:15:11.183Z] executing ['bash', '-cx', 'cd /builds/worker/checkouts/gecko/ && cp -r /build/node_modules_eslint node_modules && ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules && ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules && ./mach lint -l eslint -f treeherder --quiet\n']
[task 2019-02-28T20:15:11.185Z] + cd /builds/worker/checkouts/gecko/
[task 2019-02-28T20:15:11.185Z] + cp -r /build/node_modules_eslint node_modules
[task 2019-02-28T20:15:11.453Z] + ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules
[task 2019-02-28T20:15:11.454Z] + ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules
[task 2019-02-28T20:15:11.455Z] + ./mach lint -l eslint -f treeherder --quiet
[task 2019-02-28T20:15:12.208Z] New python executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python2.7
[task 2019-02-28T20:15:12.208Z] Also creating executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python
[task 2019-02-28T20:15:13.855Z] Installing setuptools, pip, wheel...done.
[task 2019-02-28T20:15:14.900Z] running build_ext
[task 2019-02-28T20:15:14.900Z] building 'psutil._psutil_linux' extension
[task 2019-02-28T20:15:14.900Z] creating build
[task 2019-02-28T20:15:14.900Z] creating build/temp.linux-x86_64-2.7
[task 2019-02-28T20:15:14.900Z] creating build/temp.linux-x86_64-2.7/psutil
[task 2019-02-28T20:15:14.900Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2019-02-28T20:15:14.900Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2019-02-28T20:15:14.900Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o
[task 2019-02-28T20:15:14.900Z] creating build/lib.linux-x86_64-2.7
[task 2019-02-28T20:15:14.900Z] creating build/lib.linux-x86_64-2.7/psutil
[task 2019-02-28T20:15:14.900Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so
[task 2019-02-28T20:15:14.900Z] building 'psutil._psutil_posix' extension
[task 2019-02-28T20:15:14.900Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2019-02-28T20:15:14.900Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2019-02-28T20:15:14.900Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2019-02-28T20:15:14.900Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2019-02-28T20:15:14.900Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2019-02-28T20:15:14.900Z]
[task 2019-02-28T20:15:14.901Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2019-02-28T20:20:56.876Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/content/widgets/tree.js:866:5 | Block must not be padded by blank lines. (padded-blocks)
[taskcluster 2019-02-28 20:20:57.214Z] === Task Finished ===
[taskcluster 2019-02-28 20:20:57.214Z] Unsuccessful task run with exit code: 1 completed in 620.121 seconds

Flags: needinfo?(mkmelin+mozilla)
Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bea39e11e1a8 Backed out changeset 465d08059933 for ES failure in /builds/worker/checkouts/gecko/toolkit/content/widgets/tree.js:866:5 CLOSED TREE

Here's the specific failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=231080794. Should be fixable via ./mach eslint toolkit/content/widgets/tree.js --fix.

Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f41b8dd43c MozTree event handlers should be attached in connectedCallback() instead of constructor. r=bgrins

I went ahead and re-pushed with the lint fix.

Flags: needinfo?(mkmelin+mozilla)

Thanks Brian!

https://hg.mozilla.org/mozilla-central/rev/b9f41b8dd43c

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: