DevTools: when toggling the "Highlight painted area" tool, the icon color is wrong

VERIFIED FIXED in Firefox 41


4 years ago
8 months ago


(Reporter: jsnajdr, Assigned: jsnajdr)



Firefox Tracking Flags

(firefox38 unaffected, firefox39 unaffected, firefox40 affected, firefox41 verified)


(Whiteboard: [testday-20150821][regression from bug 1128988])


(1 attachment, 1 obsolete attachment)



4 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150528030206

Steps to reproduce:

1. Open DevTools and enable the "Highlight painted area" tool.
2. Click on the "Highlight painted area" toolbar button.
3. The function is turned on and the icon gets blue.
4. Click again to turn highlighting off.

Actual results:

- highligthing is turned off, that's OK
- but the icon remains blue, as if the tool was still active
- the <toolbarbutton> still has attribute checked="true"

Expected results:

- icon is toggled back to disabled state, has black color
- the <toolbarbutton> is not in "checked" state

Comment 1

4 years ago
I can reproduce this in FDE and Nightly, but not Beta 39.0 or Release 38.0.1. Disabling e10s in Nightly and restarting made no difference.

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:40.0) Gecko/20100101 Firefox/40.0
Component: Untriaged → Developer Tools: Framework
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Component: Developer Tools: Framework → Developer Tools: Graphic Commandline and Toolbar

Comment 2

4 years ago
2015-03-29: works properly.

2015-03-30 → 2015-04-23: with e10s, clicking the button does nothing besides flooding the Browser Console with errors; without e10s, it works properly.

2015-04-24: the button now works with e10s again, but exhibits the bug described in this report, whether e10s is enabled or not.
Keywords: regression

Comment 3

4 years ago
I've got the same problem on `41.0a1 (2015-06-02)` without e10s enabled.

Comment 4

4 years ago
Working on this, please assign to me.

Comment 5

4 years ago
(In reply to Jarda Snajdr [:jsnajdr] from comment #4)
> Working on this, please assign to me.

Thank you.
Assignee: nobody → jsnajdr


4 years ago

Comment 6

4 years ago
Here is a patch. The bug was caused by incorrect handling of paintflashing_server command output - an Output object was treated as boolean.

Added a test that check the correct state.isChecked() status after every change.
Attachment #8616613 - Flags: review?(jwalker)
Attachment #8616613 - Flags: review?(jwalker) → review+
Reminder to push to TRY when the tree re-opens.
Flags: needinfo?(pbrosset)

Comment 9

4 years ago
The test browser/devtools/shared/test/browser_telemetry_button_paintflashing.js fails after my patch.

The DEVTOOLS_PAINTFLASHING_TIME_ACTIVE_SECONDS is an array that should have 2 entries, each containing for how long the tool has been active. Because the toggle button has been clicked 4 times.

However, it contains only one record, because the check happens immediately after calling the last click(), without waiting for the async action to be finished. The ACTIVE_SECONDS record is added only after the tool is closed and the active time can be computed.

Before my patch, the telemetry results didn't contain any ACTIVE_SECONDS record at all (which the test fails to notice), so no test on it was really performed. That was caused by the bug being fixed - the paintflashing tool always reported its state as "opened". The computation of active time never starts.

To fix:
- instead of calling setTimeout, we should wait for some event telling us that the click action was finished and we can check the telemetry results. But which one?
- test that ACTIVE_SECONDS record is always present.
- it's very suspicious that method checkResults is called before stopRecordingTelemetryLogs. We should check after the recording is finished.
Flags: needinfo?(pbrosset)

Comment 11

4 years ago
Fixed the browser_telemetry_button_paintflashing.js test:
- wait for the CLI command to be finished before doing anything else - using a commandOutputManager as used also in browser_telemetry_button_eyedropper.js
- rewrite the delayedClicks function into a generator function - much better code without nested promises

Try run:
Attachment #8621543 - Flags: review?(jwalker)


4 years ago
Attachment #8616613 - Attachment is obsolete: true


4 years ago
Flags: needinfo?(pbrosset)
Attachment #8621543 - Flags: review?(jwalker) → review+


4 years ago
Keywords: checkin-needed
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
QA Whiteboard: [good first verify]
I found this issue in Nightly 41.0a1 (2015-05-29) (Build ID: 20150529030205) on Linux x64

This Bug is now Verified as fixed on Latest Firefox Beta 41.0b3

Build ID : 20150820142145
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
QA Whiteboard: [good first verify] → [good first verify][testday-20150821]
Whiteboard: [testday-20150821]

Comment 15

4 years ago
I have reproduced this bug in Nightly 41.0a1 (2015-05-29)(Build ID:20150529030205) with Comment 0's instruction on Windows 10 64bit.

Bug is fixed now on latest Beta 41.0b3 (Build ID:20150820142145)

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0

As it is verified on Linux and Windows (Comment 14 and Comment 15), Marking it as verified!

Comment 17

3 years ago
This is regression from bug 1128988 (between 2015-04-23 and 2015-04-24),
and it wasn't completely fixed (see bug 1209149). Regression range:
Blocks: 1128988
See Also: → 1209149
Whiteboard: [testday-20150821] → [testday-20150821][regression from bug 1128988]


11 months ago
Product: Firefox → DevTools


8 months ago
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.