Rulers and Measure buttons should have a visible active state

RESOLVED FIXED in Firefox 66

Status

defect
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: fvsch, Assigned: yulia)

Tracking

(Blocks 1 bug, {regression})

unspecified
Firefox 66
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 unaffected, firefox65 unaffected, firefox66 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
The Rulers and Measure features have on/off states, but their buttons do not show an "active" state (with a blue icon), unlike other buttons: Paint Flashing, RDM, Frames, Pick an Element.

We should use the same "active" style to provide users with visual feedback that the feature is enabled.

Somewhat related: bug 1260225 "Move inspector-related command buttons (rulers, measurement) to the Inspector panel and enable them by default"
If we land bug 1260225 first, then we should make sure we use active states there.
(Reporter)

Updated

4 months ago
See Also: → 1260225
(Reporter)

Updated

4 months ago
Blocks: 1444299

Comment 1

4 months ago
Weird, I'm pretty sure they worked at some point. Can you try finding a regression range using https://mozilla.github.io/mozregression/ ?

EDIT: done in comment 3

Comment 2

4 months ago
Seems to work on beta, so it must be a recent regression.

Comment 3

4 months ago
12:02.30 INFO: Narrowed inbound regression window from [69649540, ae0f6d53] (3 builds) to [69649540, 7bbdead1] (2 builds) (~1 steps left)
12:02.30 INFO: No more inbound revisions, bisection finished.
12:02.30 INFO: Last good revision: 69649540a938d1f3d9f145f34d074932a3feff4e
12:02.30 INFO: First bad revision: 7bbdead1fa31f67c086340991707b62160b7c66a
12:02.30 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=69649540a938d1f3d9f145f34d074932a3feff4e&tochange=7bbdead1fa31f67c086340991707b62160b7c66a

Hi Yulia, your patch (bug 1508660) seems to cause this regression. Could you please look into this ?
Blocks: 1508660
Flags: needinfo?(ystartsev)

Comment 4

4 months ago
Not sure if that helps, but this is the code that sets the checked state for the rulers/measure command buttons: https://searchfox.org/mozilla-central/rev/ecf61f8f3904549f5d65a8a511dbd7ea4fd1a51d/devtools/client/definitions.js#596-609
(Assignee)

Comment 5

4 months ago
This fixes an issue with the buttons and also adds a test
(Assignee)

Comment 6

4 months ago
The issue is related to some of the fission work that is going on. I forgot that inspector is still a special case when updating this code. The fix was trivial, but there will be follow up work once we fix the inspector destruction issue (mentioned in the todo of the patch).

One thing I am not too sure about is how I handled the tests. Since we are not using redux in this part of the code, it makes it difficult to detect changes to the UI. Clicking the button doesn't result in a reflow right away. I used a mutation observer until we migrate this bit of the code. We might have a better pattern -- if anyone here has an idea. I will also ask around.
Flags: needinfo?(ystartsev)

Comment 7

3 months ago
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b70afc7d2c99
fix rulers and measure buttons to display state; r=jdescottes

Comment 8

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b70afc7d2c99
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Assignee: nobody → ystartsev
You need to log in before you can comment on or make changes to this bug.