Closed Bug 1169667 Opened 9 years ago Closed 8 years ago

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


(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

41 Branch
Not set


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

Firefox 41
Tracking Status
firefox38 --- unaffected
firefox39 --- unaffected
firefox40 --- affected
firefox41 --- verified


(Reporter: jsnajdr, Assigned: jsnajdr)



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


(1 file, 1 obsolete file)

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
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
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
I've got the same problem on `41.0a1 (2015-06-02)` without e10s enabled.
Working on this, please assign to me.
(In reply to Jarda Snajdr [:jsnajdr] from comment #4)
> Working on this, please assign to me.

Thank you.
Assignee: nobody → jsnajdr
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)
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)
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)
Attachment #8616613 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Attachment #8621543 - Flags: review?(jwalker) → review+
Keywords: checkin-needed
Closed: 8 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]
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!
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]
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.