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

VERIFIED FIXED in Firefox 48

Status

()

Firefox
Preferences
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Loic, Assigned: xidorn)

Tracking

({regression})

44 Branch
Firefox 48
regression
Points:
---

Firefox Tracking Flags

(firefox45 wontfix, firefox46 wontfix, firefox47 affected, firefox48 verified, firefox49 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=44904f46bc865b5a6272842e520252171ea4e404&tochange=0136029e390e363e61540205b9902dcb9f3cb3ec
Blocks: 1137009
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox48: --- → affected
Depends on: 1214064
Flags: needinfo?(bugzilla)
Keywords: regression

Updated

2 years ago
status-firefox45: affected → wontfix
status-firefox46: affected → wontfix
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)
(Assignee)

Comment 5

2 years ago
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)
(Reporter)

Comment 6

2 years ago
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.
(Assignee)

Comment 8

2 years ago
Created attachment 8742609 [details]
MozReview Request: Bug 1264968 part 1 - Use replace uses of removed String.contains with the new .includes method. r=jaws

Review commit: https://reviewboard.mozilla.org/r/47351/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47351/
Attachment #8742609 - Flags: review?(jaws)
(Assignee)

Comment 9

2 years ago
Created attachment 8742610 [details]
MozReview Request: Bug 1264968 part 2 - Allow persisting attributes of xul:window if its owner document is not root. r?enn

Review commit: https://reviewboard.mozilla.org/r/47353/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47353/
(Assignee)

Comment 10

2 years ago
(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>
(Assignee)

Comment 13

2 years ago
(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...
(Assignee)

Comment 14

2 years ago
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.
(Assignee)

Comment 16

2 years ago
(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)
(Assignee)

Comment 18

2 years ago
(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.
(Assignee)

Comment 19

2 years ago
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)
(Assignee)

Comment 21

2 years ago
Created attachment 8742644 [details]
MozReview Request: Bug 1264968 part 2 - Use <dialog> rather than <window> for pref subdialogs. r?jaws

Review commit: https://reviewboard.mozilla.org/r/47391/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47391/
Attachment #8742644 - Flags: review?(jaws)
(Assignee)

Updated

2 years ago
Attachment #8742610 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 24

2 years ago
Created attachment 8744176 [details]
MozReview Request: Bug 1264968 part 2 - Allow persisting attributes of xul:window if its owner document is not root. r?enn

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)
(Assignee)

Comment 25

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8742644 - Attachment is obsolete: true

Comment 26

2 years ago
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+
(Assignee)

Comment 27

2 years ago
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

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3663fe230f72
https://hg.mozilla.org/mozilla-central/rev/660b322ef9fd
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
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]
status-firefox49: --- → verified
QA Whiteboard: [bugday-20150518] → [bugday-20160518]
QA Whiteboard: [bugday-20160518] → [bugday-20160518][good first verify]

Comment 30

2 years ago
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]

Comment 31

2 years ago
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]
status-firefox48: fixed → verified
You need to log in before you can comment on or make changes to this bug.