The default bug view has changed. See this FAQ.

When adding a new account, the Mail Account Setup panel is too narrow

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jorg K (GMT+1), Assigned: xidorn)

Tracking

({regression})

Trunk
regression
Points:
---

Firefox Tracking Flags

(firefox43+ unaffected, firefox44 unaffected)

Details

MozReview Requests

()

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8669534 [details]
Screenshot showing the panel

When adding a new account, the Mail Account Setup panel is too narrow.
The window doesn't have scrollbars and is not resizeable.

Too narrow in recently compiled local build, see screenshot.
Looks OK in TB 41 beta.

How did that go broken?

Comment 1

2 years ago
Yes, I can see it too on Win XP.
On Win 10 too. And with every click on "Retest" the window grows vertically.
(Reporter)

Updated

2 years ago
Keywords: regression
OS: Windows 7 → Windows
Last good: Daily from 23.9.
First bad: Daily from 24.9.

I see nothing on c-c check-ins which could affect this. so probably a m-c issue.
(Reporter)

Comment 4

2 years ago
Well, where is the panel defined? Do we define a size? It's funny that all elements are on the panel, but the "Cancel" button is barely visible.

Did you check the M-C checkins for that date range?
The size should be dynamic to expand when needed.

Wayne, do you know the magic link to get the needed check-ins in a time range?
Flags: needinfo?(vseerror)
(Reporter)

Comment 6

2 years ago
http://hg.mozilla.org/comm-central/pushloghtml?startdate=2015-09-22&enddate=2015-09-25
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2015-09-22&enddate=2015-09-25
(modify as required)

You can add +HH%3AMM where HH is the hour and MM are the minutes, for example:
http://hg.mozilla.org/comm-central/pushloghtml?startdate=2015-09-22&enddate=2015-09-25+04%3A00
Flags: needinfo?(vseerror)
(Reporter)

Comment 7

2 years ago
Confirming the regression range:
Last good:
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2015-09-23-03-02-22-comm-central/
File date 11:50 UTC

First bad:
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2015-09-24-03-02-03-comm-central/
File date 12:04 UTC

So the range should be:
C-C
http://hg.mozilla.org/comm-central/pushloghtml?startdate=2015-09-23+11:50&enddate=2015-09-24+12:04
Nothing there.

M-C
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2015-09-23+11:50&enddate=2015-09-24+12:04
Sadly an awful lot there.

Looking for XUL we see bug 1137009 which deals with window sizes.

Xidorn Quan and Neil Deakin: Could it be that you caused this regression here?

Neil (Rashbrook), I'm NI'ing you since you commented on bug 1137009.
Flags: needinfo?(quanxunzhen)
Flags: needinfo?(neil)
Flags: needinfo?(enndeakin)
(Reporter)

Updated

2 years ago
Flags: needinfo?(Jan.Varga)
(Assignee)

Comment 8

2 years ago
Probably, but unlikely.

That bug changes how attributes on XUL windows are persisted. But it seems to me the window in question does not use the "persist" attribute at all (I cannot find any mention of that attribute in either emailWizard.xul or emailWizard.js), so I think it shouldn't be affected.
Flags: needinfo?(quanxunzhen)
(Reporter)

Comment 9

2 years ago
Thank you for looking at it. Wouldn't it be the "autoconfigWizard"? I looked too and didn't see anything to do with persistent.

However, looking at your changes at
https://hg.mozilla.org/mozilla-central/rev/7a5322e5fa07
and without being able to analyse them in detail, I have the impression that changes affect padding (PAD_POSITION | PAD_SIZE | PAD_MISC), which is what we are seeing here.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #8)
> Probably, but unlikely.

You mean, "possible", but unlikely?

In the regression range this change is the only one that mentions XUL, and the XUL change made does affect padding.

How can we progress? Can we bisect this further? If you can give me a patch to back out the changes, I can compile that.
Flags: needinfo?(quanxunzhen)
(Assignee)

Comment 10

2 years ago
(In reply to Jorg K (GMT+2) from comment #9)
> Thank you for looking at it. Wouldn't it be the "autoconfigWizard"? I looked
> too and didn't see anything to do with persistent.

I searched "autoconfigWizard" but only see it is mentioned in "emailWizard".

> However, looking at your changes at
> https://hg.mozilla.org/mozilla-central/rev/7a5322e5fa07
> and without being able to analyse them in detail, I have the impression that
> changes affect padding (PAD_POSITION | PAD_SIZE | PAD_MISC), which is what
> we are seeing here.

Hmmm, yes, that patch makes it unconditionally save the window geometry to attributes of the root element. If the window size can be changed via setting those attributes, then it could be affected.
(Reporter)

Comment 11

2 years ago
I will back it out locally and see what happens.
(Reporter)

Comment 12

2 years ago
Created attachment 8670178 [details] [diff] [review]
Backout changesets 2c77e4edc2a5 and 7a5322e5fa07 from bug 1137009

Backing out
https://hg.mozilla.org/mozilla-central/rev/7a5322e5fa07
https://hg.mozilla.org/mozilla-central/rev/2c77e4edc2a5
from bug 1137009 re-establishes the correct behaviour.

I think we continue in bug 1137009.
Flags: needinfo?(neil)
Flags: needinfo?(enndeakin)
Flags: needinfo?(Jan.Varga)
(Assignee)

Updated

2 years ago
Blocks: 1137009
(In reply to Xidorn Quan from comment #10)
> Hmmm, yes, that patch makes it unconditionally save the window geometry to
> attributes of the root element. If the window size can be changed via
> setting those attributes, then it could be affected.

It confuses sizeToContent() which isn't expecting those attributes to be there, and takes them to be the inner dimensions of the window. (Which would be wrong even if they were.)
(Assignee)

Comment 14

2 years ago
I'm totally scared of the undocumented XUL code.

(In reply to neil@parkwaycc.co.uk from comment #13)
> It confuses sizeToContent() which isn't expecting those attributes to be
> there, and takes them to be the inner dimensions of the window. (Which would
> be wrong even if they were.)

Some part of the code relies on that the attributes are synced with the window state, while if we actively sync those attributes unconditionally, some code gets confused. So it seems the "persist" attribute has far more side effect than "basically a convenient storage system for attribute values" you said in bug 1137009 comment 32.

We probably should figure out why SaveAttributes() is called even if the settings are not loaded from the XULStore, and try to not trigger it in that case.

If we failed to figure out that, we can probably just restore the checks on "persist" attribute in SaveAttributes() I removed in bug 1137009 part 1, although I really don't like those checks because they look inefficient.
Flags: needinfo?(quanxunzhen)

Comment 15

2 years ago
> It confuses sizeToContent() which isn't expecting those attributes to be
> there, and takes them to be the inner dimensions of the window. (Which would
> be wrong even if they were.)

Of course! The width and height are treated as box sizing attributes but they use the inner size rather than the outer size. There's another bug I think on that. 

You'll need to restore the persist checks at least for width and height.

> although I really don't like those checks because they look inefficient.

Setting the attributes unconditionally can be much less efficient as a reflow may be triggered.
(Assignee)

Comment 16

2 years ago
OK, then I'll restore that part of code. Will submit a patch tomorrow.
Assignee: nobody → quanxunzhen
Component: Account Manager → XUL
OS: Windows → All
Product: Thunderbird → Core
Hardware: x86 → All
Would this affect both 44 and 43?
status-firefox43: --- → affected
tracking-firefox43: --- → +
(Reporter)

Comment 18

2 years ago
Sure, since it got uplifted to Aurora (FF 43).
(Assignee)

Comment 19

2 years ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #17)
> Would this affect both 44 and 43?

It doesn't actually affect Firefox... We haven't seen any regression from the Firefox side.
(Assignee)

Comment 20

2 years ago
Created attachment 8670571 [details]
MozReview Request: Bug 1211344 - Stop setting various window attributes when they are not in 'persist' attribute.

Bug 1211344 - Stop setting various window attributes when they are not in 'persist' attribute.
Attachment #8670571 - Flags: review?(enndeakin)
(Assignee)

Comment 21

2 years ago
Note that, I didn't bring this code back:
>  if (persistString.IsEmpty()) { // quick check which sometimes helps
>    mPersistentAttributesDirty = 0;
>    return NS_OK;
>  }
because it seems to me the old code always wants to persist the sizemode even if it is not listed in the persist attribute.

If it eventually turns out that we really depend on this tricky optimization, I could add that code as well.
(Reporter)

Comment 22

2 years ago
Thanks for the quick turnaround. (Those Australians are ahead of everyone else ;-)).

I have applied the patch in a local build of Thunderbird and the panel in question has returned to normal operation. I can't add an f+ to any attachment here, so take this comment as f+.

I'm sure Neil can comment on the code change in detail.

Updated

2 years ago
Attachment #8670571 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 23

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e28dde22ac26e864405c1e4d0e6a5524bfe7aeba
Bug 1211344 - Stop setting various window attributes when they are not in 'persist' attribute. r=enndeakin
https://hg.mozilla.org/mozilla-central/rev/e28dde22ac26
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Reporter)

Comment 25

2 years ago
Comment on attachment 8670571 [details]
MozReview Request: Bug 1211344 - Stop setting various window attributes when they are not in 'persist' attribute.

Approval Request Comment
[Feature/regressing bug #]: bug 1137009

Please uplift this to Aurora since the patch that broke it in bug 1137009 was also uplifted to Aurora.
Attachment #8670571 - Flags: approval-mozilla-aurora?
Comment on attachment 8670571 [details]
MozReview Request: Bug 1211344 - Stop setting various window attributes when they are not in 'persist' attribute.

OK, approved for uplift since I don't want us to break Thunderbird.
Hard to evaluate the risk here.
Attachment #8670571 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 27

2 years ago
Thank you. Please allow me the following comments:

XUL is a Mozilla *key* technology. It went broken caused by bug 1137009. We don't know which other repercussions this would have had (for example to add-ons).

There is no risk: In bug 1137009 the behaviour was changed to a non-working state. It has now been restored. Not shipping this fix in FF 43 would have been a *huge* risk.
Jorg:  Well, yes, but if we're fixing something, then usually we can also break it even more. 

So, please explain your reasoning about user impact, risk, test coverage, and so on, in your uplift requests. Thanks!
(Reporter)

Comment 29

2 years ago
Liz: Sorry, my explanation in the uplift request was too short. Here it comes now.

The correct code was:
  if (such-and-such-condition) {
    do X;
  }

and that was erroneously replaced by:
  do X; // always do X

Now it's restored to the original state
  if (such-and-such-condition) {
    do X;
  }

This does actually not break anything and there is no risk involved. This is what happened here, please trust the developers and the code review on this.

The proper way would have been to *back out* bug 1137009, then we wouldn't have this discussion here. Since backing out stuff always comes with some hassle, it was decided to quickly undo the damage in this bug here. Consequently this has to be landed on Aurora.

Sadly there doesn't appear to be test coverage for this, so personally, I would have required this bug to be landed with a new test to ensure that the functionality doesn't break in the future.
(Reporter)

Updated

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

Comment 30

2 years ago
Comment on attachment 8670571 [details]
MozReview Request: Bug 1211344 - Stop setting various window attributes when they are not in 'persist' attribute.

OK, let me fill the form for Jorg K

Approval Request Comment
[Feature/regressing bug #]: bug 1137009
[User impact if declined]: Some XUL window may have unexpected size
[Describe test coverage new/current, TreeHerder]: not aware of any
[Risks and why]: this change itself should have pretty low risk because it is merely a partial backout of bug 1137009
[String/UUID change made/needed]: n/a
https://hg.mozilla.org/releases/mozilla-aurora/rev/2fbc5d0584e4
status-firefox43: affected → fixed
Backed out in 
https://hg.mozilla.org/integration/mozilla-inbound/rev/9db45b599d6a
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ee2ce4e485f
Status: RESOLVED → REOPENED
status-firefox43: fixed → affected
status-firefox44: fixed → affected
Resolution: FIXED → ---
Target Milestone: mozilla44 → ---
(Assignee)

Comment 33

2 years ago
This is a regression of bug 1137009, and that bug has been backed out as well, so this bug should not be reopened.
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox43: affected → unaffected
status-firefox44: affected → unaffected
Resolution: --- → FIXED
Duplicate of this bug: 1212618
Duplicate of this bug: 1215704
You need to log in before you can comment on or make changes to this bug.