Closed Bug 1264968 Opened 8 years ago Closed 8 years ago

Sizes from many windows in about:preferences are not saved after the user resized and closed them

Categories

(Firefox :: Settings UI, defect)

44 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 --- affected
firefox48 --- verified
firefox49 --- verified

People

(Reporter: epinal99-bugzilla2, Assigned: xidorn)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Maybe a dupe of bug 1214064.

STR:
1) Open about:preferences#security
2) Click on "Saved Logins"
3) Resize and close the window
4) Click again on "Saved Logins"

Result: the window has its default size.

Many windows of about:preferences are affected, the user should be able to keep a size of his choice for these windows, especially for saved logins. That was always the case in the past.
I think this is actually a front-end bug, about:preferences is in-content now, so I'd expect the front-end code there would need to explicitly save the pseudo-window sizes. (This happened automatically for XUL windows, but we're not using those when it's in-content.).
Component: Widget → Preferences
Flags: needinfo?(jaws)
Product: Core → Firefox
(How is bug 1137009 even involved here? Do we believe the regression range?)
Bug 1043346 cleared the sizes of dialogs when they were closed (Firefox 37) so one resized subdialog wouldn't affect the size of the next opened subdialog even if they were different subdialogs. Then bug 1043612 and bug 1153403 persisted the sizes of dialogs when they were closed (Firefox 40) so the next time the same subdialog is reopened it would reopen at the previous size.

In bug 1043612 we used XUL attribute persistence to store these. Regrettably that patch didn't introduce an automated test.
Flags: needinfo?(jaws)
Given comment 4, I know how to fix it. But I don't have time to investigate how to write the test, so if anyone can help here, it would be appreciated.
Assignee: nobody → bugzilla
Flags: needinfo?(bugzilla)
The funny thing is some windows are not affected.
In about:preferences#security, the first "Exceptions" window is affected (size not saved) but not the 2nd "Exceptions" window.
You may be able to look at /browser/components/preferences/in-content/tests/browser_subdialogs.js to see how tests can open subdialogs. Note that browser_subdialogs.js uses a generic subdialog, not the ones that are actually opened in the real preferences.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> You may be able to look at
> /browser/components/preferences/in-content/tests/browser_subdialogs.js to
> see how tests can open subdialogs. Note that browser_subdialogs.js uses a
> generic subdialog, not the ones that are actually opened in the real
> preferences.

Then it is actually not testable. This issue happens because dialogs in preferences use <xul:window> for its content.

I manually tested the patchset, and it seems it works.
Comment on attachment 8742609 [details]
MozReview Request: Bug 1264968 part 1 - Use replace uses of removed String.contains with the new .includes method. r=jaws

https://reviewboard.mozilla.org/r/47351/#review44059

A test here would have prevented this from silently breaking. Did you take a look at browser_subdialogs.js?
Attachment #8742609 - Flags: review?(jaws) → review+
Sorry I didn't see comment 10. That doesn't explain the switch from .contains to .includes though. We could also change subdialog.xul to use a <xul:window>
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Sorry I didn't see comment 10. That doesn't explain the switch from
> .contains to .includes though.

Part 2 is the real fix, but part 1 probably also involves since it stops setting width/height attributes unconditionally.

> We could also change subdialog.xul to use a <xul:window>

Hmmm, probably...
But why was the test designed to use <dialog> from the very first?
Some of our "dialogs" use <dialog>, see http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/preferences/changemp.xul#16. The difference has been negligible to this point and I believe <dialog> was chosen because of semantics and not expecting behavioral differences.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)
> Some of our "dialogs" use <dialog>, see
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/preferences/
> changemp.xul#16. The difference has been negligible to this point and I
> believe <dialog> was chosen because of semantics and not expecting
> behavioral differences.

Well, then I guess another way to fix this issue is, changing every <window> to <dialog>, then. Since only <window> was broken on persist due to the assumption that it is only used for top level window.

That also says, even if there was a test, the test would not catch this regression either.
Matt, do you recall why <dialog> was used for the test?
Flags: needinfo?(MattN+bmo)
(In reply to Loic from comment #6)
> The funny thing is some windows are not affected.
> In about:preferences#security, the first "Exceptions" window is affected
> (size not saved) but not the 2nd "Exceptions" window.

The second "Exceptions" window uses <prefwindow> rather than <window>. It seems there are roughly same number of <window> and <prefwindow> uses. Probably we can consider converting all <window> to <prefwindow> to fix this issue rather than change the C++ code.
It seems <prefwindow> and <window> does have some behavioral differences.

:jaws, could you take this bug and try fixing it via converting all <window> on pref subdialogs to something different? <dialog> and <prefwindow> should both work. I'm not quite familiar with all these xul things... And I suppose making them consistent is a good thing itself.
Flags: needinfo?(jaws)
I don't have the time right now to make and test that change. Some dialogs may be opened outside of the preferences in SeaMonkey or other programs.
Flags: needinfo?(jaws)
Attachment #8742610 - Attachment is obsolete: true
No longer depends on: 1214064
Comment on attachment 8742644 [details]
MozReview Request: Bug 1264968 part 2 - Use <dialog> rather than <window> for pref subdialogs. r?jaws

https://reviewboard.mozilla.org/r/47391/#review44169

The Saved Logins dialog is also reachable from right-clicking on a login form and selecting Fill Login > View Saved Logins. This change adds an OK/Cancel to that dialog, http://screencast.com/t/sUZ1F0u1
Attachment #8742644 - Flags: review?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> Matt, do you recall why <dialog> was used for the test?

Probably like you said because it made sense semantically but I don't remember off-hand. It seems like we should be testing all of the types of documentElements that we use in prefs.
Flags: needinfo?(MattN+bmo)
Review commit: https://reviewboard.mozilla.org/r/48363/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48363/
Attachment #8742609 - Attachment description: MozReview Request: Bug 1264968 part 1 - Use replace uses of removed String.contains with the new .includes method. r?jaws → MozReview Request: Bug 1264968 part 1 - Use replace uses of removed String.contains with the new .includes method. r=jaws
Attachment #8744176 - Flags: review?(enndeakin)
Comment on attachment 8742609 [details]
MozReview Request: Bug 1264968 part 1 - Use replace uses of removed String.contains with the new .includes method. r=jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47351/diff/1-2/
Attachment #8742644 - Attachment is obsolete: true
Comment on attachment 8744176 [details]
MozReview Request: Bug 1264968 part 2 - Allow persisting attributes of xul:window if its owner document is not root. r?enn

https://reviewboard.mozilla.org/r/48363/#review45173
Attachment #8744176 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3663fe230f72ac37afbc25996cca6f2ef8794a4d
Bug 1264968 part 1 - Use replace uses of removed String.contains with the new .includes method. r=jaws

https://hg.mozilla.org/integration/mozilla-inbound/rev/660b322ef9fd22614359864cb2ebff3943511766
Bug 1264968 part 2 - Allow persisting attributes of xul:window if its owner document is not root. r=enndeakin
https://hg.mozilla.org/mozilla-central/rev/3663fe230f72
https://hg.mozilla.org/mozilla-central/rev/660b322ef9fd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
I have reproduced this bug on Nightly 48.0a1 (2016-04-15) on ubuntu 16.04 LTS, 64 bit!

The bug's fix is now verified on Latest Nightly 49.0a1!

Build ID: 20160518030234
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
QA Whiteboard: [bugday-20150518]
QA Whiteboard: [bugday-20150518] → [bugday-20160518]
QA Whiteboard: [bugday-20160518] → [bugday-20160518][good first verify]
Reproduced this bug with Firefox nightly 48.0a1(build id:20160424030601)on
windows 7(64 bit)

Verified this bug as fixed with Firefox beta 48.0b3(build id:20160623122823)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

Verified as fixed with Firefox aurora 49.0a2(build id:20160723004004)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160722]
As the bug is verified on firefox 48, firefox 49 by linux(Comment 29) and windows(Comment 30) marking as verified
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20160518][good first verify] → [bugday-20160518][good first verify][testday-20160722]
You need to log in before you can comment on or make changes to this bug.