Closed Bug 1517187 Opened 2 years ago Closed 2 years ago

Rulers and Measure buttons should have a visible active state

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

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

RESOLVED FIXED
Firefox 66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 --- unaffected
firefox66 --- fixed

People

(Reporter: fvsch, Assigned: yulia)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

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.
See Also: → 1260225
Blocks: 1444299
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
Seems to work on beta, so it must be a recent regression.
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)
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
This fixes an issue with the buttons and also adds a test
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)
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b70afc7d2c99
fix rulers and measure buttons to display state; r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/b70afc7d2c99
Status: NEW → RESOLVED
Closed: 2 years 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.