994729 - Make box model guides move on layout view region hover

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The layout view guides are supposed to outline the hovered region but currently don't.

This one liner fixes it.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: layout-view-guides.patch

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

try:
https://tbpl.mozilla.org/?tree=Try&rev=7289d7427fbc

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8404736 [details] [diff] [review]
layout-view-guides.patch

Review of attachment 8404736 [details] [diff] [review]:
-----------------------------------------------------------------

Do we have tests for this?

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Added test

(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.

Comment 6

[Brian Grinstead [:bgrins]]

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");

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: layout-view-guides-994729.patch

(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.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: layout-view-guides-994729.patch

Fixed broken test

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: layout-view-guides-994729.patch

Fixed issue with test failing when tests run as a batch.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=ee49f2d885d9

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: layout-view-guides-994729.patch

https://hg.mozilla.org/integration/fx-team/rev/49bf1fa2a369

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49bf1fa2a369