Closed
Bug 994729
Opened 11 years ago
Closed 10 years ago
Make box model guides move on layout view region hover
Categories
(DevTools :: Inspector, defect, P1)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 33
People
(Reporter: miker, Assigned: miker)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
miker
:
review+
|
Details | Diff | Splinter Review |
The layout view guides are supposed to outline the hovered region but currently don't.
This one liner fixes it.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #8404736 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 2•11 years ago
|
||
Comment 3•11 years ago
|
||
Comment on attachment 8404736 [details] [diff] [review]
layout-view-guides.patch
Review of attachment 8404736 [details] [diff] [review]:
-----------------------------------------------------------------
Do we have tests for this?
Attachment #8404736 -
Flags: review?(bgrinstead) → review+
Updated•11 years ago
|
Blocks: firebug-gaps
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #3)
> Comment on attachment 8404736 [details] [diff] [review]
> layout-view-guides.patch
>
> Review of attachment 8404736 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Do we have tests for this?
It does now but we will need to leave the test disabled as it is waiting on Patrick's reflow actor so that we can stop using MozAfterPaint events in the layout view. MozAfterPaint events break layoutview-updated causing intermittent oranges and leaks.
All the layout view tests are currently disabled for this reason.
Attachment #8404736 -
Attachment is obsolete: true
Attachment #8407547 -
Flags: review?(bgrinstead)
Comment 6•11 years ago
|
||
Comment on attachment 8407547 [details] [diff] [review]
Added test
Review of attachment 8407547 [details] [diff] [review]:
-----------------------------------------------------------------
I get a leak when running the test locally: TEST-UNEXPECTED-FAIL | browser/devtools/layoutview/test/browser_layoutview_guides.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/layoutview/view.xhtml]
If this is related to what you said in comment 5, then we need to make sure that it is skipped in the browser.ini file and do a push to try before landing - unless if the whole folder is ignored elsewhere.
::: browser/devtools/layoutview/test/head.js
@@ +13,5 @@
> Services.prefs.setIntPref("devtools.toolbox.footer.height", 350);
> gDevTools.testing = true;
> SimpleTest.registerCleanupFunction(() => {
> Services.prefs.clearUserPref("devtools.inspector.sidebarOpen");
> Services.prefs.clearUserPref("devtools.toolbox.footer.height");
We should be clearing the added pref in the cleanup function:
Services.prefs.clearUserPref("devtools.dump.emit");
Attachment #8407547 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #6)
> Comment on attachment 8407547 [details] [diff] [review]
> Added test
>
> Review of attachment 8407547 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I get a leak when running the test locally: TEST-UNEXPECTED-FAIL |
> browser/devtools/layoutview/test/browser_layoutview_guides.js | leaked 2
> window(s) until shutdown [url =
> chrome://browser/content/devtools/layoutview/view.xhtml]
>
All the layout view tests do... I believe that is why they are disabled.
> If this is related to what you said in comment 5, then we need to make sure
> that it is skipped in the browser.ini file and do a push to try before
> landing - unless if the whole folder is ignored elsewhere.
>
I believe all of the layout view tests are ignored but a try run is a good idea:
https://tbpl.mozilla.org/?tree=Try&rev=7c2d8e9115bb
> ::: browser/devtools/layoutview/test/head.js
> @@ +13,5 @@
> > Services.prefs.setIntPref("devtools.toolbox.footer.height", 350);
> > gDevTools.testing = true;
> > SimpleTest.registerCleanupFunction(() => {
> > Services.prefs.clearUserPref("devtools.inspector.sidebarOpen");
> > Services.prefs.clearUserPref("devtools.toolbox.footer.height");
>
> We should be clearing the added pref in the cleanup function:
>
> Services.prefs.clearUserPref("devtools.dump.emit");
Removed the pref, we don't need it.
Attachment #8407547 -
Attachment is obsolete: true
Attachment #8407609 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Summary: Layout view guides not moving on hover → Make box model guides move on layout view region hover
Assignee | ||
Comment 8•11 years ago
|
||
Fixed broken test
Attachment #8407609 -
Attachment is obsolete: true
Attachment #8434185 -
Flags: review+
Assignee | ||
Comment 9•11 years ago
|
||
Fixed issue with test failing when tests run as a batch.
Try:
https://tbpl.mozilla.org/?tree=Try&rev=ee49f2d885d9
Attachment #8434185 -
Attachment is obsolete: true
Attachment #8436882 -
Flags: review+
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8436882 -
Attachment is obsolete: true
Attachment #8445260 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Whiteboard: [fixed-in-fx-team]
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•