Conversation
| } | ||
| } | ||
|
|
||
| // set event listener on "show url" buttons |
There was a problem hiding this comment.
This seems a bit complex, although presumably it would be possible to replace all of them with focus or click. (If not, I suspect you need focus in addition to the rest.)
It seems more intuitive to use the input event on the text/url input.
There was a problem hiding this comment.
focus is made for the purpose, mousedown and keydown is not. It's a more complex way of doing more or less the same thing hopefully identically and potentially slightly worse. mousedown and keydown should not be used in this way.
I do not understand why it would be better to use the input event
More correctly stated, the blur, change or input event.
You have three different behaviors:
- mouseover to update preview, but this introduces a lot of insecurities, like what does tap & hold, copy link on a mobile browser do in terms of events, how buggy are they, could we get a flash of outdated URL
- focus to update right before the actual click action
- click to check if the URL is empty and focus the input field
But why bother with the hours of time spent testing assistive technologies and mobile browsers when you can just update the DOM on input?
Additionally if you don't do it on input it won't work as intended with the links panel or with link selectors, or bookmarklets such as these.
There was a problem hiding this comment.
I changed to click event.
|
|
||
| // set event listener on "show url" buttons | ||
| function init_url_observers() { | ||
| document.querySelectorAll('.open-url').forEach(function (btn) { |
There was a problem hiding this comment.
Not super important because it is not our main page, but a loop to add event listeners is a bad sign. It is lighter to add a single event listener and use "event delegation" https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Events#event_delegation
Several examples in e.g. this function:
Lines 777 to 779 in da2adae
There was a problem hiding this comment.
It's also not uncommon to just set it all on document (with a similar event.target/event.target.matches/event.target.closest pattern). The overhead from a bit of if/else or switch or whatever is significantly less than adding an eventlistener on a gazillion elements,particularly in memory. But yeah, I doubt it matters here.
There was a problem hiding this comment.
it is the same like the show password buttons.
Co-authored-by: Alexandre Alapetite <[email protected]>

The show the website button is improved now:

Changes proposed in this pull request:
How to test the feature manually:
Pull request checklist: