Closed
Bug 398289
Opened 17 years ago
Closed 17 years ago
persist="selectedIndex" no longer working for tabbox elements
Categories
(Core :: XBL, defect)
Core
XBL
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b1
People
(Reporter: mozbgz, Assigned: mayhemer)
References
(Depends on 1 open bug)
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
(deleted),
application/vnd.mozilla.xul+xml
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
With this change to nsPresShell.cpp, a regression for tabbox was introduced:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsPresShell.cpp&branch=&root=/cvsroot&subdir=mozilla/layout/base&command=DIFF_FRAMESET&rev1=3.1056&rev2=3.1057
> Process XBL constructors after processing style reresolves. In particular,
> this makes sure that we process the former even if there were none of the
> latter. Bug 394676 and bug 394014
Since 2007-09-14, persist="selectedIndex" is broken for tabbox elements (as demonstrated by cert manager - see bug 373525): when the tabbox is opened, it will always show the content of the first tab. Switching between two tabs will correctly refresh the content, however.
(To reproduce the problem, use a recent trunk build, open cert manager from the security preferences ["View Certificates"], select one of the tabs 2..5, close cert manager and reopen - it will always open with the contents of "Your Certificates".)
Comment 1•17 years ago
|
||
Can you create a testcase that's smaller than the entire cert manager, by any chance? That would really help with getting this fixed...
(In reply to comment #1)
> Can you create a testcase that's smaller than the entire cert manager, by any
> chance?
Yes, here is one, which can be tested in any browser window (I've tried recent trunk builds from both Firefox and Seamonkey).
To easily reproduce the effect, select the second tab ("Two") and then reload the document - while the "Two" tab will stay selected, its content is then replaced by that of tab ONE. The same happens if you select tab two, change to another URL and then come back.
An interesting thing to note is that it's actually the presence of the treecol element on the first tab which triggers the bug - as soon as this one is removed, the problem will disappear...
Finally - fwiw, removing the
mDocument->BindingManager()->ProcessAttachedQueue();
line from nsPresShell.cpp would "fix the problem", though that's probably not what you want to do, I assume.
Comment 3•17 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007100303 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
Checking testcase with this nightly (and new profile),
which is before "2007-10-03 10:21 / kaie%kuix.de / Bug 373525" and "2007-10-03 16:38 / bzbarsky%mit.edu / Bug 394676":
no bug, persistence works fine.
But I get errors like
[
Error: uncaught exception: Permission denied to get property XPCComponents.classes
Security Error: Content at chrome://global/content/bindings/tree.xml may not load data from https://bugzilla.mozilla.org/attachment.cgi?id=283367.
Error: this.view has no properties
Source File: chrome://global/content/bindings/tree.xml
Line: 0
]
which may be other bugs !?
Comment 4•17 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007100304 Minefield/3.0a9pre] (nightly) (W2Ksp4)
Same behavior: testcase works and getting
[
Error: uncaught exception: Permission denied to get property
XPCComponents.classes
]
Status: NEW → ASSIGNED
Updated•17 years ago
|
Status: ASSIGNED → NEW
(In reply to comment #3)
> Checking testcase with this nightly (and new profile),
> which is before "2007-10-03 10:21 / kaie%kuix.de / Bug 373525" and "2007-10-03
> 16:38 / bzbarsky%mit.edu / Bug 394676":
> no bug, persistence works fine.
Sigh. Yes, you're correct - it was fixed by this commit to nsPresShell.cpp (on 2007-10-01):
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsPresShell.cpp&branch=&root=/cvsroot&subdir=mozilla/layout/base&command=DIFF_FRAMESET&rev1=3.1063&rev2=3.1064
> But I get errors like
> [
> Error: uncaught exception: Permission denied to get property
> XPCComponents.classes
>
> Security Error: Content at chrome://global/content/bindings/tree.xml may not
> load data from https://bugzilla.mozilla.org/attachment.cgi?id=283367.
>
> Error: this.view has no properties
> Source File: chrome://global/content/bindings/tree.xml
> Line: 0
> ]
> which may be other bugs !?
Mostly cosmetic - they will disappear if you load the test case as a chrome URI (try adding the file to toolkit.jar, with content/global/test-398289.xul as its relative pathname, and then open chrome://global/content/test-398289.xul).
Boris, how "safe" is it to resolve this bug? Any other upcoming changes to nsPresShell in the next few days, which might reintroduce this regression...? ;-)
Comment 6•17 years ago
|
||
Well, I'd just gotten that same un-regression range and was wondering what patch caused it... ;)
The reason that patch fixed the bug is that without it, creating the nsTreeColFrame under that ContentInserted call that InitialReflow makes was causing a layout flush (from nsTreeBoxObject::GetTreeBody, called by nsTreeBoxObject::GetColumns, called by nsTreeColFrame::InvalidateColumns), which ran XBL constructors, so the constructor of the tabbox ran too early, I presume, before all the necessary things were in place. So its selectedIndex munging failed.
The change to not allow flushing under there was purposeful, so I would certainly hope it won't regress! But just to make sure, it would be great to turn this testcase into a regression test... I wonder whether it would be doable using reftest with the XUL in a big enough iframe.
Kaspar, do you want to give that a shot, or should I?
Depends on: 398108
Flags: in-testsuite?
(In reply to comment #6)
> But just to make sure, it would be great to
> turn this testcase into a regression test... I wonder whether it would be
> doable using reftest with the XUL in a big enough iframe.
>
> Kaspar, do you want to give that a shot, or should I?
I'll gladly leave that to you, since I'm not very experienced in all the XUL/XBL/reftest stuff, so you're welcome to take over ;-) Thanks!
So it sounds like this is fixed? If not, feel free to renominate.
Flags: blocking1.9?
Comment 9•17 years ago
|
||
Checked in a reftest for this.
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Reporter | ||
Comment 10•17 years ago
|
||
(In reply to comment #9)
> Checked in a reftest for this.
Thanks! In the meantime I learned at least how to *run* reftests and was curious to see if it would really fail for trunk builds from 2007-09-15 to 2007-09-01... and in the current version it does not, unfortunately.
What needs to be added is setting the selectedIndex to 0 either after 398289-1.html is unloaded (window.onunload), or when 398289-1-ref.html is loaded.
"Like so", actually ;-)
Index: 398289-1-ref.html
===================================================================
RCS file: /cvsroot/mozilla/layout/reftests/bugs/398289-1-ref.html,v
retrieving revision 1.2
diff -u -r1.2 398289-1-ref.html
--- 398289-1-ref.html 6 Oct 2007 05:21:03 -0000 1.2
+++ 398289-1-ref.html 8 Oct 2007 17:39:04 -0000
@@ -3,6 +3,7 @@
<head>
<script>
window.onload = function () {
+ window.frames[0].document.getElementById("test").selectedIndex = 0;
window.frames[0].document.getElementById("test").selectedIndex = 1;
document.documentElement.className = "";
}
Boris, can you add this change (or should I add a patch as an attachment, because it needs reviews/approval)?
Comment 11•17 years ago
|
||
Hmm. It was failing over here... But I wasn't loading it in the test-then-ref order, I guess. Thank you for checking on this!
I checked in that change.
Assignee | ||
Comment 12•16 years ago
|
||
This is mochitest chrome variant of the reftest framework. I tested with rollback patch for this bug (expected failure) and also with patch for bug 295994 (expected success).
Attachment #338692 -
Flags: review?(dbaron)
Updated•16 years ago
|
Attachment #338692 -
Flags: review?(bzbarsky)
Comment 13•16 years ago
|
||
Comment on attachment 338692 [details] [diff] [review]
Fixed test
bzbarsky would be a better reviewer for this
Comment 14•16 years ago
|
||
Comment on attachment 338692 [details] [diff] [review]
Fixed test
> <html class="reftest-wait" style="height: 100%">
Ditch the class, and the className set below.
>+ ok(equal, "persistent attribute in tab box broken");
>+
>+ dump("\n\n\n"+str1+"\n\n\n"+str2+"\n\n\n");
Put the URIs in the string for the ok() call instead of adding a separate dump.
With that, looks good.
Attachment #338692 -
Flags: review?(dbaron)
Attachment #338692 -
Flags: review?(bzbarsky)
Attachment #338692 -
Flags: review+
Assignee | ||
Comment 15•16 years ago
|
||
Attachment #338692 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 16•16 years ago
|
||
Comment on attachment 340540 [details] [diff] [review]
Fixed test2
[Checkin: Comment 16]
http://hg.mozilla.org/mozilla-central/rev/cfe2d978cab1
Attachment #340540 -
Attachment description: Fixed test2 → Fixed test2
[Checkin: Comment 16]
Updated•16 years ago
|
Attachment #338692 -
Flags: review+
Comment 17•16 years ago
|
||
(In reply to comment #9)
> Checked in a reftest for this.
Which one ?
Comment 18•16 years ago
|
||
The one that someone later went and removed.
Comment 19•16 years ago
|
||
Serge, for what it's worth the checkin comment on your checkin was wrong: you were just checking in a different test for this bug, not a fix for it.
You need to log in
before you can comment on or make changes to this bug.
Description
•