Closed
Bug 1172357
Opened 9 years ago
Closed 9 years ago
about:welcomeback tree is broken
Categories
(Firefox :: Migration, defect)
Firefox
Migration
Tracking
()
VERIFIED
FIXED
Firefox 42
Tracking | Status | |
---|---|---|
firefox38 | --- | unaffected |
firefox39 | + | wontfix |
firefox38.0.5 | --- | unaffected |
firefox40 | + | verified |
firefox41 | + | verified |
firefox42 | --- | verified |
People
(Reporter: ntim, Assigned: ntim)
References
Details
(Keywords: regression, Whiteboard: [bugday-20150729])
Attachments
(1 file, 1 obsolete file)
8.96 KB,
patch
|
jaws
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
[Tracking Requested - why for this release]: recent regression Screenshot : http://cl.ly/image/07351M1E1K1Z
Assignee | ||
Comment 1•9 years ago
|
||
So, the about:welcomeback page needed the same treatment as about:sessionrestore. The reason we have to introduce nesting with .tree-container is simply because applying flexbox properties on the tree directly breaks the XUL tree.
Attachment #8616497 -
Flags: review?(jaws)
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Attachment #8616497 -
Flags: review?(jaws) → review?(dao)
Comment 2•9 years ago
|
||
Tim can you request uplift to aurora and beta when this is ready to go? We are going to build with 39 beta 6 on Monday morning. Thanks.
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 3•9 years ago
|
||
This patch needs to be reviewed before landing.
Flags: needinfo?(ntim.bugs)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(dao)
Comment 4•9 years ago
|
||
How bad is this problem? It doesn't sound like something we should break. Does this make the page unusable? Should this block the 39 release?
Flags: needinfo?(ntim.bugs)
Comment 5•9 years ago
|
||
Justin, what is your opinion on this? Do you think this is critical for 39? Thanks
Flags: needinfo?(dolske)
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Liz Henry (:lizzard) from comment #4) > How bad is this problem? It doesn't sound like something we should break. > Does this make the page unusable? Should this block the 39 release? This is rather bad for about:welcomeback. Although, given the low usage of the page, this bug is less critical. This page is displayed after resetting Firefox. Also, if an urgent fix is needed, we could simply back out bug 1125952 from beta.
Flags: needinfo?(ntim.bugs)
Comment 7•9 years ago
|
||
I'm not sure backing out bug 1125952 would be much of an improvement. Can you walk me through what about:welcomeback does? I did not want to try to read up on this and reproduce it as it has been a very busy week with travel, working through the weekend, and I'm tracking many other bugs for this release. Does this mean possible data loss for someone who hits the welcome back page and then can't restore their data? What is actually broken? What are they supposed to see or be able to do, and what do they see instead? How many people do hit this -- ie what do you mean by low usage? Thanks!
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Liz Henry (:lizzard) from comment #7) > I'm not sure backing out bug 1125952 would be much of an improvement. I really have no preference here. If you consider this bug critical enough, that solution is possible. > Can you walk me through what about:welcomeback does? I did not want to try > to read up on this and reproduce it as it has been a very busy week with > travel, working through the weekend, and I'm tracking many other bugs for > this release. Does this mean possible data loss for someone who hits the > welcome back page and then can't restore their data? What is actually > broken? What are they supposed to see or be able to do, and what do they > see instead? about:welcomeback shows up when the user resets Firefox. With this bug, the user will be unable to select specific tabs/windows to restore. The user will see http://cl.ly/image/07351M1E1K1Z if he/she attempts to choose specific tabs/windows to restore. > How many people do hit this -- ie what do you mean by low usage? Thanks! I don't have specific numbers, but given that resetting Firefox isn't something you do every day, not much people will hit into this issue. Not much people know about this feature too.
Flags: needinfo?(ntim.bugs)
Comment 9•9 years ago
|
||
Comment on attachment 8616497 [details] [diff] [review] Patch > function updateTabListVisibility() { >- let tabList = document.getElementById("tabList"); >+ let tabList = document.querySelector(".tree-container"); >+ let container = document.querySelector(".container"); > if (document.getElementById("radioRestoreChoose").checked) { > tabList.setAttribute("available", "true"); >+ container.classList.add("flex"); > } else { > tabList.removeAttribute("available"); >+ container.classList.remove("flex"); > } Please rename the "flex" class to something that makes more sense semantically and explains its higher-level purpose. On a related note, do we support having multiple "containers" in a document? If not, container should be an id rather than a class. Depending on how much that class is already used, that might be something for a separate bug... >-#tabList { >+.tree-container { > display: none; > } > >-#tabList[available] { >+.tree-container[available] { > display: -moz-box; > } .tree-container is a div, I don't think we want -moz-box for that. Here's what you probably should replace the above with: .tree-container:not([available]) { display: none; }
Flags: needinfo?(dao)
Attachment #8616497 -
Flags: review?(dao) → review-
Comment 10•9 years ago
|
||
Wontfix for 39. We should aim to get this into 40 once it's in shape though.
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #9) > Comment on attachment 8616497 [details] [diff] [review] > Patch > > > function updateTabListVisibility() { > >- let tabList = document.getElementById("tabList"); > >+ let tabList = document.querySelector(".tree-container"); > >+ let container = document.querySelector(".container"); > > if (document.getElementById("radioRestoreChoose").checked) { > > tabList.setAttribute("available", "true"); > >+ container.classList.add("flex"); > > } else { > > tabList.removeAttribute("available"); > >+ container.classList.remove("flex"); > > } > > Please rename the "flex" class to something that makes more sense > semantically and explains its higher-level purpose. Does flex-content sound good to you ? > On a related note, do we support having multiple "containers" in a document? > If not, container should be an id rather than a class. Depending on how much > that class is already used, that might be something for a separate bug... I agree. It's probably safer to do this later though since we use the stylesheet in 4 pages already. Filed bug 1177245. > >-#tabList { > >+.tree-container { > > display: none; > > } > > > >-#tabList[available] { > >+.tree-container[available] { > > display: -moz-box; > > } > > .tree-container is a div, I don't think we want -moz-box for that. Here's > what you probably should replace the above with: > > .tree-container:not([available]) { > display: none; > } Will do.
Comment 12•9 years ago
|
||
(In reply to Tim Nguyen [:ntim] (ltd. availability 17-25 June) from comment #11) > (In reply to Dão Gottwald [:dao] from comment #9) > > Comment on attachment 8616497 [details] [diff] [review] > > Patch > > > > > function updateTabListVisibility() { > > >- let tabList = document.getElementById("tabList"); > > >+ let tabList = document.querySelector(".tree-container"); > > >+ let container = document.querySelector(".container"); > > > if (document.getElementById("radioRestoreChoose").checked) { > > > tabList.setAttribute("available", "true"); > > >+ container.classList.add("flex"); > > > } else { > > > tabList.removeAttribute("available"); > > >+ container.classList.remove("flex"); > > > } > > > > Please rename the "flex" class to something that makes more sense > > semantically and explains its higher-level purpose. > Does flex-content sound good to you ? The main issue with "flex" or "flex-content" is that the name is tightly coupled with the CSS properties it currently uses. For example, a class name of "white-border" would be out of date once the design spec says that the border should be red. Does ".restore-chosen" make sense?
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8616497 -
Attachment is obsolete: true
Flags: needinfo?(dolske)
Attachment #8628237 -
Flags: review?(dao)
Assignee | ||
Updated•9 years ago
|
Attachment #8628237 -
Flags: review?(dao) → review?(jaws)
Updated•9 years ago
|
Attachment #8628237 -
Flags: review?(jaws) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8628237 [details] [diff] [review] Patch v2 Approval Request Comment [Feature/regressing bug #]: bug 1125952 [User impact if declined]: Broken about:welcomeback functionality [Describe test coverage new/current, TreeHerder]: Will land on m-c, tested locally [Risks and why]: medium, contains a few JS changes, but mostly renaming selectors [String/UUID change made/needed]: nope
Attachment #8628237 -
Flags: approval-mozilla-beta?
Attachment #8628237 -
Flags: approval-mozilla-aurora?
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/879c46e60e5c
Keywords: checkin-needed
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/879c46e60e5c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Comment 17•9 years ago
|
||
This fix has just landed on m-c. Flagging QE to verify the fix before it is uplifted.
Flags: qe-verify?
Flags: needinfo?(florin.mezei)
Comment 18•9 years ago
|
||
Verified as fixed using the following environment: FF 42 Build Id: 20150720030213 OS: Win 7 x64, Mac Os X 10.9.5, Ubuntu 12.04 x86 The issue described in this bug has been fixed. During testing we encountered bug 1114565 (Refreshing Firefox twice in a row will not delete the old profile and will not restart Firefox). When trying to refresh the second time Firefox failes to restart and the following error is displayed: "Your Firefox profile cannot be loaded. It may be missing or inaccessible."
Comment 19•9 years ago
|
||
Comment on attachment 8628237 [details] [diff] [review] Patch v2 This fix has been verified by QE. (Thanks for the quick turnaround!) Let's get this into beta6. Beta+ Aurora+
Attachment #8628237 -
Flags: approval-mozilla-beta?
Attachment #8628237 -
Flags: approval-mozilla-beta+
Attachment #8628237 -
Flags: approval-mozilla-aurora?
Attachment #8628237 -
Flags: approval-mozilla-aurora+
Comment 22•9 years ago
|
||
Setting this for quick verification in Firefox 40 Beta.
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Comment 23•9 years ago
|
||
Successfully reproduce the bug on 41.0a1 (2015-06-07) (Build ID: 20150607030217) on Linux x64. This Bug is now verified as fixed on Latest Aurora 41.0a2 (2015-07-29) (Build ID: 20150729004002) and Latest Firefox Beta 40.0b7 (Build ID: 20150727174134) User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0 User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0
QA Whiteboard: [bugday-20150729]
Whiteboard: [bugday-20150729]
Comment 24•9 years ago
|
||
Thanks Nazir! I also did some quick verification today on Windows 7 x64, Mac OS X 10.9.5, and Ubuntu 14.04 x64, using Firefox 40 Beta 8 (BuildID=20150727174134) and the latest Firefox 41 DevEdition (BuildID=20150729004002), and the issue no longer showed. Note that for DevEdition I had to use the dev-edition-default profile for the Refresh button to show in about:support (due to bug 1100334).
You need to log in
before you can comment on or make changes to this bug.
Description
•