1888170 - Hide reset button until user makes changes in color themes menu

Status: NEW

(Toolkit :: Reader Mode, enhancement, P5)

Description

[Irene Ni]

non-blocker level bug for rolling out new theme and layout menus.

As a usability improvement, we want to hide the "Reset Defaults" button in the color themes menu until the user has make changes to one of the custom colors, since at the moment it is clickable/enabled at all times. This can be done through a pref that is flipped on when there are any custom colors set, and is off initially or if the user resets.

We can also revisit the button styling with design, since it is currently styled to look like a link even though it functions as a button. For the moment, the link styling has been approved by the design systems team in their design review, so this is an improvement rather than a blocker-level bug.

Comment 1

[[:alan]]

Hi, first time contributor here. I'm hoping to eventually take this issue - I've set up my dev environment and am currently reading the code. Just wanted to confirm that the code in question is located in /toolkit/components/reader? Thanks in advance for any help.

Comment 2

[Irene Ni]

Hi Alan, welcome! You're correct, the reader view code is in /toolkit/components/reader. I see earlier that I suggested we could use another pref, but an easier way could be to check if there are custom values when we set up the color inputs, and show/hide the reset-button initially based on that. Then, you could use the event listener in _setupColorInput to update the button once the user picks a color. Some suggestions if you want to try this out:

1. I would create a method _toggleResetButton that toggles the reset button style to display: block when you want it to show, or display: none if not.
2. Add a boolean variable in _SetupCustomColors that tracks if any custom colors exist in the prefs. You could check the value of the color inputs against the value for the corresponding property in DEFAULT_COLORS, and if any of them don't match, you know a custom color exists. You can call _toggleResetButton using the boolean to determine whether to display on initial load.
3. Modify the event listener in _setupColorInput so that once a color-picked event occurs, the reset button is shown.
4. Reorder _setupButton to go above _setupCustomColors so that the reset-button is set up before we set its display value.

Comment 3

[[:alan]]

Attachment: Bug 1888170 - [toolkit] reset default button is now hidden until user changes colors. r=ini

Comment 4

[[:alan]]

Hi Irene,

Thanks for the helpful suggestions! I have submitted a patch on Phabricator. For future reference, where is the best place to communicate with reviewers during this process (e.g. bugzilla, matrix)?

Comment 5

[Irene Ni]

Hi Al! Thanks for working on this, I'll review it soon. For questions specific to the bug, you can comment on the patch in Bugzilla. If you have questions about the reader view code or want help with troubleshooting, you can ask the matrix channel.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.