Closed Bug 1336072 Opened 3 years ago Closed 3 years ago

[compact layout] Rearranging and deleting tiles are broken

Categories

(Firefox :: New Tab Page, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(1 file)

We use 'top' and 'left' for drag feedback and for rearranging tiles when removing one. position:absolute on #newtab-grid breaks this.
Why didn't the automated tests catch this? We have tests for this...
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?
Attachment #8832846 - Flags: review?(gijskruitbosch+bugs)
(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 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
Attachment #8832846 - Flags: review+
(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?
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9cd704d6900c
Avoid setting position:absolute on #newtab-grid since it breaks rearranging and deleting tiles. r=Gijs
(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.
https://hg.mozilla.org/mozilla-central/rev/9cd704d6900c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Flags: in-testsuite?
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]: /
Attachment #8832846 - Flags: approval-mozilla-beta?
Attachment #8832846 - Flags: approval-mozilla-aurora?
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.
Attachment #8832846 - Flags: approval-mozilla-beta?
Attachment #8832846 - Flags: approval-mozilla-beta+
Attachment #8832846 - Flags: approval-mozilla-aurora?
Attachment #8832846 - Flags: approval-mozilla-aurora+
CC'ing Grover, just to make sure he's in the loop here.
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]
You need to log in before you can comment on or make changes to this bug.