Closed Bug 1038997 Opened 5 years ago Closed 5 years ago

Backout newtab changes including layout and styling changes for beta

Categories

(Firefox :: New Tab Page, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox31 + verified
firefox32 + verified
firefox-esr31 + verified

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(5 files, 3 obsolete files)

For 31 currently in beta, we want to backout all newtab related changes to avoid bug 1000097
For the latest attempt, I reverted newtab related directories to just before this commit:

* a682dda - Bug 895359 - Switch about:newtab to Promise.jsm and remove remaining callback r=jaws (6 months ago) <Tim Taubert>

git di --binary a682dda^ browser/{base/content{,/test},themes/{linux,osx,windows}}/newtab | git apply -R

Then added back in various commits:
https://github.com/Mardak/tiles-dev/pull/40


Tests are failing because drag/drop events aren't being synthesized correctly:
https://tbpl.mozilla.org/?tree=Try&rev=ff4cda08a684
https://tbpl.mozilla.org/php/getParsedLog.php?id=43899728&tree=Try
uncaught exception - TypeError: win is null at chrome://mochitests/content/browser/browser/base/content/test/newtab/head.js:465
https://hg.mozilla.org/try/file/a557e47d7c26/browser/base/content/test/newtab/head.js#l465
   462 function synthesizeNativeMouseEvent(aElement, aMsg, aOffsetX = 0, aOffsetY = 0) {

   463   let rect = aElement.getBoundingClientRect();

   464   let win = aElement.ownerDocument.defaultView;

   465   let x = aOffsetX + win.mozInnerScreenX + rect.left + rect.width / 2;
So backing out newtab related changes didn't seem to work and backing out even more still didn't work, so I tried to revert just bug 991210 bug 980014 bug 978338 and patch up search by making the search-form flex 5 and padding it on both sides with <div class="newtab-side-margin"/>.

https://tbpl.mozilla.org/?tree=Try&rev=b5e6c1fce07a
https://github.com/Mardak/tiles-dev/pull/41
Attached image 30 Release screenshot
With the backedout beta keeping the search bar, the scrollbars start appearing at a bigger height. See attached overlapped view of 31 vs 30.
Attached patch v2Splinter Review
There were 4 steps involved in this patch:
- Revert Bug 980014
- Revert Bug 978338
- Revert Bug 991210
- Fix up search form 
https://github.com/Mardak/tiles-dev/pull/41/commits
Assignee: nobody → edilee
Attachment #8456500 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8457026 - Flags: review?(adw)
Attached patch release-to-backout.patch (obsolete) — Splinter Review
Not sure if this helps, but these are the changes to browser newtab code since Release.
Attachment #8457026 - Flags: review?(adw) → review+
Attachment #8457026 - Flags: approval-mozilla-release?
Attachment #8457026 - Flags: approval-mozilla-release? → approval-mozilla-release+
(In reply to Ed Lee :Mardak from comment #9)
> https://hg.mozilla.org/releases/mozilla-release/rev/2552ea57100d

Setting the status flag to fixed so it shows up in our RC2 verification query. 

Also marking ESR status flag since this needs to land for 31esr AFAIK.
Attached patch for aurora (obsolete) — Splinter Review
I cherry-picked the patch to aurora, but bc1 is failing:
https://tbpl.mozilla.org/?tree=Try&rev=17ae17d2bcc7

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tabopen_reflows.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tabopen_reflows.js | application timed out after 330 seconds with no output
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tabopen_reflows.js | application terminated with exit code 5
PROCESS-CRASH | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tabopen_reflows.js | application crashed [@ libSystem.B.dylib + 0xd7a]
Attachment #8457027 - Attachment is obsolete: true
Actually, the test failures in comment 12 seem to be caused by bug 1039881.
No longer blocks: 1000097
Comment on attachment 8458055 [details] [diff] [review]
for aurora

Approval Request Comment
[Feature/regressing bug #]: revert bug 980014, bug 978338
[User impact if declined]: users will see a new tiles layout in 32 then a different one in 33
[Describe test coverage new/current, TBPL]: https://tbpl.mozilla.org/?tree=Try&rev=0768f53e2850
[Risks and why]: low risk - already landed for 31rc2
[String/UUID change made/needed]: none
Attachment #8458055 - Flags: approval-mozilla-aurora?
Attachment #8458055 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: cornel.ionce
Verified that the mentioned newtab changes were successfully backed-out from:
1. Firefox 31 RC build 2 (build ID: 20140716183446)
2. Firefox 31 ESR (build ID: 20140717132905).

Changing the tracking flags accordingly. I will leave the "verifyme" keyword untouched until all branches will be fixed and verified.
This bug is clear of the tart crash: https://tbpl.mozilla.org/?tree=Try&rev=9a0ba6ff126f
Attachment #8458055 - Attachment is obsolete: true
Whiteboard: checkin-needed-aurora
Fixed for both 31 (beta -> release) and 32 (aurora -> beta)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Verified fixed on Firefox 32.0a2 Aurora, build ID: 20140721004001.
I'll keep an eye on this fix to make sure it landed properly in Firefox 31 release and Firefox 32 beta.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Target Milestone: --- → Firefox 31
This landed properly in Firefox 31 release (build ID: 20140716183446) and Firefox 32 beta 1 (build ID: 20140722195627).
You need to log in before you can comment on or make changes to this bug.