1504254 - Add ability to modify preferences with a double click in the new about:config

Status: RESOLVED FIXED

(Toolkit :: Preferences, enhancement, P3)

Comment 1

[:Paolo Amadini]

This P5 bug was originally filed as investigation because the old "about:config" page supported this interaction, even if it was not part of the original design discussion.

Now that the implementation is complete, we can see that this interaction does not fit well with the new user interface model. The page now looks like a standard in-content page, and on those the standard behavior of a double click is to extend the selection, which is a useful interaction by itself to select preference names or values. The selection can be copied, opened as a link, or searched for in a search engine.

We now have a clear, visible button for toggling the preference with a single click instead. I ran this new page past a few people, who noticed the difference but didn't find any difficulty with using the toggle button instead of double clicking.

We're also explicitly supporting the standard triple click to select an entire name or value, and we don't want to accidentally toggle preferences while using this feature.

Comment 3

[:Paolo Amadini]

Given the UX refresh in bug 1533856, we should implement the double click to toggle or edit, and remove the triple click interaction while keeping drag to select.

Comment 4

[Filip Š]

I've done a fix for this bug. I'm not familiar with Mercurial so I don't know how to create and submit a patch.
Is this possible from Phabricator or some other web interface?

Comment 5

[:Paolo Amadini]

Filip, before tackling this bug, I suggest you look for others marked as "good first bugs" in your area of interest on https://codetribute.mozilla.org/.

You can use one of those to familiarize yourself with how to format a patch, run existing automated tests, and write new test cases. In this bug, not only the code needs to react to the double click event, but tests also have to be updated to check that text selection is handled appropriately in the double and triple click cases. Thus, it is more complex than it looks at first sight.

Comment 6

[Filip Š]

Attachment: Bug 1504254 - Handle double and triple clicks in about:config.

Comment 7

[Filip Š]

I've created a fix and tests for it. Revision is available at https://phabricator.services.mozilla.com/D25438.

It handles both double and triple clicks.
When a row is double-clicked, the edit button will automatically be clicked which will trigger editing. Also, any selection will be cleared. When a row is triple-clicked, the code will do nothing so a selection of name or value will be made.

Comment 8

[:Paolo Amadini]

Thanks! It's a start, but the next step is probably to read the surrounding code and familiarize yourself with all the functions that already exist in our codebase to achieve the same result with less code, and the techniques we used. As I mentioned already, this bug may take more time than average, but if you're up for it we appreciate the contribution!

There are a few examples of thing that are done differently here. In specific cases we use constants, instead of hard-coding timeouts. The ESLint output also hints at how we avoid setTimeout in tests. Things like children[0] that depend on current structure should also be avoided in production code. And finally, we place one event listener on the entire table instead of one per row or element, to save memory and processing time - there is one for clicks already.

It may be at least two weeks before I can do a more detailed code review on this. Matthias, maybe you are interested in providing some feedback to Filip, since you're already familiar with the design of this code?

Mike, it's also not clear to me if we plan to have someone assigned to outstanding dependencies of bug 1493439. Is the plan to keep this feature enabled on Nightly and rely on contributors to finish the work over a longer time span than a single release?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Paolo Amadini from comment #8)

Mike, it's also not clear to me if we plan to have someone assigned to outstanding dependencies of bug 1493439. Is the plan to keep this feature enabled on Nightly and rely on contributors to finish the work over a longer time span than a single release?

Hi Paolo,

The plan for now is to try to get this pushed through this quarter once a few left-over things from Q1 settle down, so we're going to keep this enabled on Nightly for a bit.

Comment 10

[Matthias :Kammueller]

Hi,
Sorry for the late reply. I'll try to take a first look at it tomorrow =)

Comment 11

[:Paolo Amadini]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #9)

The plan for now is to try to get this pushed through this quarter once a few left-over things from Q1 settle down, so we're going to keep this enabled on Nightly for a bit.

Cool! If this isn't ready for the 69 cycle, I recommend doing some manual testing of the XUL-based "about:config" page during the next code freeze, since we don't have regression tests and some of the ongoing de-XBL work might affect it.

I've previously used P3 versus P5 to mark required bugs versus optional ones, but it's likely that new optional bugs will be filed as P3 in the meantime. To keep the scope of work contained, at some point you may want to triage those bugs again, or mark some bugs as more explicit blockers of bug 1532703.

Comment 12

[:Paolo Amadini]

Hi Filip, sorry for the reduced availability on this. Just wanted to make sure, even before one of us does a more detailed review, if you would be interested in fixing some of the general issues mentioned in comment 8?

After that you may have to wait some more time, but it may provide a step forward. I know it's not the best experience, but this area of the project is still on hold, as Mike mentioned in comment 9. You're also welcome to work on more active areas, or continue the work here.

Comment 13

[Filip Š]

Ok. Can you provide some more details how to do that?

Comment 14

[:Paolo Amadini]

Sure, I can point out the affected lines in the code review, and you can then submit a new version of your patch after making the required fixes.

Comment 15

[:Paolo Amadini]

Mike and Markus, how does the 250ms delay on double click implemented in Filip's patch feel to you? I think it makes the interface seem a bit less responsive, and in some cases we may get a double click selection highlight that is then lost, but the trade-off in removing or reducing the delay too much is that we would lose the ability to use triple click to select long values precisely.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Passing the buck to aswan who's picking up this work. :)

Comment 17

[Markus Jaritz [:designakt] (UX)]

(In reply to :Paolo Amadini from comment #15)

Mike and Markus, how does the 250ms delay on double click implemented in Filip's patch feel to you? I think it makes the interface seem a bit less responsive, and in some cases we may get a double click selection highlight that is then lost, but the trade-off in removing or reducing the delay too much is that we would lose the ability to use triple click to select long values precisely.

Is there a build I could try this with? I tested in Nightly, but it doesn't seem to work there.

Comment 18

[:Paolo Amadini]

I've started tryserver builds here, they're artifact builds and should be ready soon:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01c6941b7b0fb39739bdb9132d14dc08ff2b08e9

If this doesn't work, there will be regular builds here:

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

Comment 19

[Markus Jaritz [:designakt] (UX)]

Attachment: about-config-double-click.gif

(In reply to :Paolo Amadini from comment #15)

Mike and Markus, how does the 250ms delay on double click implemented in Filip's patch feel to you?

I don't see an issue with the 250ms delay. What is annoying is the disappearing selection as it happens every single time - even if one is not clicking on text. (see attachment)

preventDefault() on mousedown-events with a click-count of 2 can stop the selection on double-click. see https://codepen.io/designakt/pen/BejZbM Let's use this to prevent the unwanted selection.

Comment 20

[:Paolo Amadini]

Filip, do you plan to address the latest review comments? I may have some time available for the next review pass in the following days.

Comment 21

[Filip Š]

Sorry, I didn't have much time this week. I'm currently working on this so I will probably fix review comments soon.

Comment 22

[Filip Š]

I addressed all review comments. This now also uses Markus' suggestion for handling selection and other suggestions in review comments.

I also tested this manually and it works and updated tests.
However, I currently can't set up build environment so I can't run tests. Can you check if they work correctly?

Comment 23

[:Paolo Amadini]

Thanks! I replied in the review comments.

Comment 24

[:Paolo Amadini]

I've asked Mark if he would like to continue working on this bug. As noted in the review, the groundwork done here is quite valuable, but the code needs clean up and new regression tests have to be written. Mark, if you're interested, let me know if you need help with parsing my past review comments.

Comment 25

[Filip Š]

Ok, thank you! Setting up a build and testing environment is very hard and I wasn't able to do this completely. So it would be good if someone else could finish my work.

Comment 26

[Mark Striemer [:mstriemer]]

Attachment: Bug 1504254 - Edit prefs on double click r?paolo

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9549e85c43e
Edit prefs on double click r=paolo

Comment 28

[Andreea Pavel [:apavel]]

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