Closed Bug 1172357 Opened 9 years ago Closed 9 years ago

about:welcomeback tree is broken

Categories

(Firefox :: Migration, defect)

defect
Not set
normal

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)

[Tracking Requested - why for this release]: recent regression

Screenshot : http://cl.ly/image/07351M1E1K1Z
Attached patch Patch (obsolete) — Splinter Review
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)
Attachment #8616497 - Flags: review?(jaws) → review?(dao)
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)
This patch needs to be reviewed before landing.
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(dao)
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)
Justin, what is your opinion on this? Do you think this is critical for 39? Thanks
Flags: needinfo?(dolske)
(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)
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)
(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 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-
Wontfix for 39. We should aim to get this into 40 once it's in shape though.
(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.
(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?
Attached patch Patch v2Splinter Review
Attachment #8616497 - Attachment is obsolete: true
Flags: needinfo?(dolske)
Attachment #8628237 - Flags: review?(dao)
Attachment #8628237 - Flags: review?(dao) → review?(jaws)
Attachment #8628237 - Flags: review?(jaws) → review+
Keywords: checkin-needed
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?
https://hg.mozilla.org/mozilla-central/rev/879c46e60e5c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
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)
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 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+
Setting this for quick verification in Firefox 40 Beta.
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
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]
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.