1336072 - [compact layout] Rearranging and deleting tiles are broken

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Dão Gottwald [:dao]]

We use 'top' and 'left' for drag feedback and for rearranging tiles when removing one. position:absolute on #newtab-grid breaks this.

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1336072 - Avoid setting position:absolute on #newtab-grid since it breaks rearranging and deleting tiles.

Review commit: https://reviewboard.mozilla.org/r/109106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109106/

Comment 2

[:Gijs (he/him)]

Why didn't the automated tests catch this? We have tests for this...

Comment 3

[:Gijs (he/him)]

Comment on attachment 8832846 [details]
Bug 1336072 - Avoid setting position:absolute on #newtab-grid since it breaks rearranging and deleting tiles.

https://reviewboard.mozilla.org/r/109106/#review110270

::: browser/themes/shared/newtab/newTab.inc.css:109
(Diff revision 1)
>    color: #7A7A7A;
>    font-size: 1em;
>    font-weight: normal;
>    /* Position the heading such that it doesn't affect how many cells we
>       can fit into the grid. */
>    position: absolute;

Does this still want to be position: absolute now that it no longer has the position: relative ancestor?

Comment 4

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #2)
> Why didn't the automated tests catch this? We have tests for this...

It still works under the hood, stuff just appears at the wrong place on the screen (if not off-screen).

> Does this still want to be position: absolute now that it no longer has the
> position: relative ancestor?

Yes.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8832846 [details]
Bug 1336072 - Avoid setting position:absolute on #newtab-grid since it breaks rearranging and deleting tiles.

https://reviewboard.mozilla.org/r/109106/#review110276

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to :Gijs from comment #2)
> > Why didn't the automated tests catch this? We have tests for this...
> 
> It still works under the hood, stuff just appears at the wrong place on the
> screen (if not off-screen).

Can we add tests for this?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9cd704d6900c
Avoid setting position:absolute on #newtab-grid since it breaks rearranging and deleting tiles. r=Gijs

Comment 8

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #4)
> > (In reply to :Gijs from comment #2)
> > > Why didn't the automated tests catch this? We have tests for this...
> > 
> > It still works under the hood, stuff just appears at the wrong place on the
> > screen (if not off-screen).
> 
> Can we add tests for this?

Should be possible in theory using getBoundingClientRect, I think, but I can't spend more time on this at the moment.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/9cd704d6900c

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8832846 [details]
Bug 1336072 - Avoid setting position:absolute on #newtab-grid since it breaks rearranging and deleting tiles.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1334444

[User impact if declined]: dragging about:newtab tiles and deleting tiles don't give proper UI feedback

[Is this code covered by automated tests?]: no

[Has the fix been verified in Nightly?]: no

[Needs manual test from QE? If yes, steps to reproduce]: should be covered as part of the next funnelcake's QA

[List of other uplifts needed for the feature/fix]: /

[Is the change risky?]: no

[Why is the change risky/not risky?]: it's behind a hidden pref (browser.newtabpage.compact)

[String changes made/needed]: /

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8832846 [details]
Bug 1336072 - Avoid setting position:absolute on #newtab-grid since it breaks rearranging and deleting tiles.

Regression from 52, should only affect users who switch default prefs, let's take it for aurora and beta.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3b80d305c75a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/51523138a95e

Comment 14

[Andrei Vaida [:avaida]]

CC'ing Grover, just to make sure he's in the loop here.

Comment 15

[Saheda Reza [:Antora]]

I have reproduced this according to Firefox nightly (2017-02-02)

Fixing bug is verified on Latest Developer Edition, Latest Beta , Latest Nightly

 -- Build ID:(20170303004003) , User Agent: Mozilla/5.0 (Windows NT 10.0; rv:53.0) Gecko/20100101 Firefox/53.0
 -- Build ID :(20170307064827),User Agent:Mozilla/5.0 (Windows NT 10.0; rv:53.0) Gecko/20100101 Firefox/53.0
 -- Build ID:(20170308030207), User Agent:Mozilla/5.0 (Windows NT 10.0; rv:55.0) Gecko/20100101 Firefox/55.0

Tested OS-- Windows10 32bit
[bugday-20170308]