Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2184123003:
Use event path to detect if anchor has been clicked in WebUIs. (Closed)
|
Created:
4 years, 4 months ago by calamity Modified:
4 years, 4 months ago CC:
chromium-reviews, oshima+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@close_dialog_on_query Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse event path to detect if anchor has been clicked in WebUIs.
This CL fixes an issue where MD History could not open file URLs due to
the way the global anchor click listener for WebUIs worked. This has been
fixed by checking the event path, as is done in iron-location's global
click handler.
BUG=621260
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/0684a6f3db61f973734e654292e00ade79e8bd5d
Cr-Commit-Position: refs/heads/master@{#410972}
Patch Set 1 : #Patch Set 2 : add test to util_test.html #
Total comments: 16
Patch Set 3 : address comments #
Total comments: 6
Patch Set 4 : address comments #
Total comments: 2
Patch Set 5 : add <b> test #
Depends on Patchset: Messages
Total messages: 36 (20 generated)
Description was changed from ========== Use event path to detect if anchor has been clicked in WebUIs. This CL fixes an issue where MD History could not open file URLs due to the way the global anchor click listener for WebUIs worked. This has been fixed by checking the event path, as is done in iron-location's global click handler. BUG=621260 ========== to ========== Use event path to detect if anchor has been clicked in WebUIs. This CL fixes an issue where MD History could not open file URLs due to the way the global anchor click listener for WebUIs worked. This has been fixed by checking the event path, as is done in iron-location's global click handler. BUG=621260 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
calamity@chromium.org changed reviewers: + tsergeant@chromium.org
+tsergeant for initial review.
It would be nice to verify in util_test.html that none of the existing behavior has changed (plus, writing tests there should be easier than in history)
On 2016/07/28 05:58:54, tsergeant wrote: > It would be nice to verify in util_test.html that none of the existing behavior > has changed (plus, writing tests there should be easier than in history) Done.
lgtm
calamity@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam for util.js
https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_list_test.js (right): https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_list_test.js:279: registerMessageCallback('navigateToUrl', this, function(info) { registerMessageCallback() is a bummer. didn't know you guys were using this in new code... https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_list_test.js:292: registerMessageCallback('navigateToUrl', this, function() {}); why is this necessary? https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/util_test.html (right): https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/util_test.html:27: chrome.send = function(message, args) { also a bummer https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/util_test.html:27: chrome.send = function(message, args) { at least replace this, IMO, after you're done https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/util_test.html:29: lastArgs = args; clickArgs or clickParams, IMO https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/util_test.html:30: } }; https://codereview.chromium.org/2184123003/diff/60001/ui/webui/resources/js/u... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/2184123003/diff/60001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:216: if (e.defaultPrevented) nit: in my ideal world if (e.defaultPrevented || e.button > 1) return; var link = e.path.find(el => el.tagName == 'A' && el.href); if (!link || !['about:', 'file:'].includes(link.protocol)) return; chrome.send('navigateToUrl', [ link.href, link.target, e.button, e.altKey, e.ctrlKey, e.metaKey, e.shiftKey ]); e.preventDefault(); BUT =>, .find(), and .includes() wont work in Safari, so bollocks https://codereview.chromium.org/2184123003/diff/60001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:219: var eventPath = e.path; this code runs on safari, which may have dodgy support for path (if any). you probably need to handle target as well or something...
https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_list_test.js (right): https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_list_test.js:279: registerMessageCallback('navigateToUrl', this, function(info) { On 2016/08/02 06:02:43, Dan Beam wrote: > registerMessageCallback() is a bummer. didn't know you guys were using this in > new code... Any alternatives? https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_list_test.js:292: registerMessageCallback('navigateToUrl', this, function() {}); On 2016/08/02 06:02:43, Dan Beam wrote: > why is this necessary? undefined would work here too. We need to tear down the message callback to prevent it from running on subsequent calls during other tests. https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/util_test.html (right): https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/util_test.html:27: chrome.send = function(message, args) { On 2016/08/02 06:02:43, Dan Beam wrote: > at least replace this, IMO, after you're done Done. https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/util_test.html:29: lastArgs = args; On 2016/08/02 06:02:43, Dan Beam wrote: > clickArgs or clickParams, IMO Done. https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/util_test.html:30: } On 2016/08/02 06:02:43, Dan Beam wrote: > }; Done. https://codereview.chromium.org/2184123003/diff/60001/ui/webui/resources/js/u... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/2184123003/diff/60001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:216: if (e.defaultPrevented) On 2016/08/02 06:02:44, Dan Beam wrote: > nit: in my ideal world > > if (e.defaultPrevented || e.button > 1) > return; > > var link = e.path.find(el => el.tagName == 'A' && el.href); > if (!link || !['about:', 'file:'].includes(link.protocol)) > return; > > chrome.send('navigateToUrl', [ > link.href, > link.target, > e.button, > e.altKey, > e.ctrlKey, > e.metaKey, > e.shiftKey > ]); > e.preventDefault(); > > BUT =>, .find(), and .includes() wont work in Safari, so bollocks Acknowledged. https://codereview.chromium.org/2184123003/diff/60001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:219: var eventPath = e.path; On 2016/08/02 06:02:43, Dan Beam wrote: > this code runs on safari, which may have dodgy support for path (if any). you > probably need to handle target as well or something... Done.
https://codereview.chromium.org/2184123003/diff/80001/ui/webui/resources/js/u... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/2184123003/diff/80001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:215: document.addEventListener('click', function(e) { can we make this a static function so we can call it from tests? and then actually write some tests :)? https://codereview.chromium.org/2184123003/diff/80001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:229: } could this work? /** * @param {!Node} node * @return {boolean} Whether |node| is an <a href>. */ function isAnchorWithHref(node) { return node.nodeName == 'A' && node.href; } var anchor = e.path ? e.path.find(isAnchorWithHref) : findAncestor(e.target, isAnchorWithHref); https://codereview.chromium.org/2184123003/diff/80001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:243: nit: anchor = /** @type {!HTMLAnchorElement} */(anchor);
https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_list_test.js (right): https://codereview.chromium.org/2184123003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_list_test.js:279: registerMessageCallback('navigateToUrl', this, function(info) { On 2016/08/08 05:06:43, calamity wrote: > On 2016/08/02 06:02:43, Dan Beam wrote: > > registerMessageCallback() is a bummer. didn't know you guys were using this > in > > new code... > > Any alternatives? don't call chrome.send() directly from your code. instead make a "BrowserProxy" thing like we do in settings https://codereview.chromium.org/2184123003/diff/80001/ui/webui/resources/js/u... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/2184123003/diff/80001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:229: } On 2016/08/08 19:29:48, Dan Beam wrote: > could this work? > > /** > * @param {!Node} node > * @return {boolean} Whether |node| is an <a href>. > */ > function isAnchorWithHref(node) { > return node.nodeName == 'A' && node.href; > } > > var anchor = e.path ? e.path.find(isAnchorWithHref) : > findAncestor(e.target, isAnchorWithHref); actually, Array#find might also suck wrt compat UGH
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2184123003/diff/80001/ui/webui/resources/js/u... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/2184123003/diff/80001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:215: document.addEventListener('click', function(e) { On 2016/08/08 19:29:47, Dan Beam wrote: > can we make this a static function so we can call it from tests? and then > actually write some tests :)? Do you mean pulling the event handler out into a static function? What other cases need testing aside from those in util_test.html? Also, will that test run on iOS? https://codereview.chromium.org/2184123003/diff/80001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:243: On 2016/08/08 19:29:47, Dan Beam wrote: > nit: anchor = /** @type {!HTMLAnchorElement} */(anchor); Done.
i thought of a few ways to golf this code smaller, but not sure if it's worth it lgtm let's just not look at util.js much harder :( https://codereview.chromium.org/2184123003/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/util_test.html (right): https://codereview.chromium.org/2184123003/diff/100001/chrome/test/data/webui... chrome/test/data/webui/util_test.html:5: <a id="chrome" href="about:chrome">Chrome</a> this doesn't handle the case of <a href="file:///blah"> <b>Click me</b><!-- click()ing this --> </a>
https://codereview.chromium.org/2184123003/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/util_test.html (right): https://codereview.chromium.org/2184123003/diff/100001/chrome/test/data/webui... chrome/test/data/webui/util_test.html:5: <a id="chrome" href="about:chrome">Chrome</a> On 2016/08/09 17:13:22, Dan Beam wrote: > this doesn't handle the case of > > <a href="file:///blah"> > <b>Click me</b><!-- click()ing this --> > </a> Added a case.
lgtm++
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org Link to the patchset: https://codereview.chromium.org/2184123003/#ps120001 (title: "add <b> test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Use event path to detect if anchor has been clicked in WebUIs. This CL fixes an issue where MD History could not open file URLs due to the way the global anchor click listener for WebUIs worked. This has been fixed by checking the event path, as is done in iron-location's global click handler. BUG=621260 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Use event path to detect if anchor has been clicked in WebUIs. This CL fixes an issue where MD History could not open file URLs due to the way the global anchor click listener for WebUIs worked. This has been fixed by checking the event path, as is done in iron-location's global click handler. BUG=621260 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/0684a6f3db61f973734e654292e00ade79e8bd5d Cr-Commit-Position: refs/heads/master@{#410972} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0684a6f3db61f973734e654292e00ade79e8bd5d Cr-Commit-Position: refs/heads/master@{#410972} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
