Last Comment Bug 398837 - [FIX]Keyboard accelerators missing on Security Warning dialog
: [FIX]Keyboard accelerators missing on Security Warning dialog
: regression, verified1.8.1.10, verified1.8.1.8
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 1.8 Branch
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Boris Zbarsky [:bz]
Depends on:
Blocks: 267833
  Show dependency treegraph
Reported: 2007-10-06 03:35 PDT by Steve England [:stevee]
Modified: 2007-11-27 10:34 PST (History)
10 users (show)
dveditz: blocking1.8.1.8+
dveditz: blocking1.8.1.10+
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Note the missing letters where the keyboard accelerators should be (6.18 KB, image/png)
2007-10-06 03:35 PDT, Steve England [:stevee]
no flags Details
Fix (6.07 KB, patch)
2007-10-07 17:15 PDT, Boris Zbarsky [:bz]
roc: review+
roc: superreview+
dveditz: approval1.8.1.8+
Details | Diff | Review

Description Steve England [:stevee] 2007-10-06 03:35:52 PDT
Created attachment 283829 [details]
Note the missing letters where the keyboard accelerators should be

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv: Gecko/2007100503 BonEcho/

1. New profile, start firefox
2. Tools > Options > Security > Warning Message > Settings

- All tickboxes should have a keyboard accelerator, and no letters should be missing.

- Only the first tickbox has a keyboard accelerator; where the others should have one there is nothing, so the sentences read silly. eg: "I ubmit information that's not encrypted" or "I eave an encrypted page for one that isn't encrypted".

Works in Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv: Gecko/2007091417 Firefox/ with a new profile.

Found by Warduke at
Comment 1 Steve England [:stevee] 2007-10-06 06:43:20 PDT
works: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv: Gecko/2007093004 BonEcho/

broken: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv: Gecko/2007100103 BonEcho/

Checkins to module AviarySuiteBranchTinderbox on branch MOZILLA_1_8_BRANCH between 2007-09-30 03:00 and 2007-10-01 04:00 :

So I will guess a regression of bug 267833, bug 373756, bug 394676 or bug 394014. CC'ng bz since he seemed to have a lot to do with those bugs.
Comment 2 Daniel Veditz [:dveditz] 2007-10-06 08:33:54 PDT
I see nearly the same (I see the 's' in the submit one as well as the first 'v' one). What is special about this dialog?

I don't see any problems on the similar-looking Advanced JavaScript options child dialog, for example. Gavin's name is in the credits here, maybe he knows something special about this dialog.

Comment 3 :Gavin Sharp [email:] 2007-10-06 15:29:43 PDT
Haven't been able to look into this deeply, but I don't know of anything special about that dialog off-hand. It certainly doesn't seem to differ from advanced-scripts.xul in any significant way (in fact I think I may have copied that file when I added this one).
Comment 4 Boris Zbarsky [:bz] 2007-10-07 14:18:04 PDT
It looks like we fail to find the primary frame for the <span> when inserting the textnode under it in <method name="wrapChar"> in text.xml.  So we never construct a textframe for that one letter, and the rest follows.

Looking into why we don't find the frame.
Comment 5 Boris Zbarsky [:bz] 2007-10-07 17:15:40 PDT
Created attachment 283948 [details] [diff] [review]

This bug is a result of two things.

First, the patch for bug 267833 changed the ordering of constructor firing and layout under InitialReflow.  As a result, without this patch on branch we do the initial layout, _then_ fire XBL constructors.  In the case of this dialog, that causes the text in the labels to wrap, so that the frame tree for one of the labels looks something like this right after we've inserted the child span node:

(nsAreaFrame *) 0x88a315c (nsXULElement *) 0x8b15cb0
  (nsInlineFrame *) 0x88a2f8c (nsHTMLSpanElement *) 0x88a4358
    (nsTextFrame *) 0x8b17b28 (nsTextNode *) 0x8b15d38
  (nsInlineFrame *) 0x8b2161c (nsHTMLSpanElement *) 0x88a4358
    (nsContinuingTextFrame *) 0x8b215d8 (nsTextNode *) 0x8b15d38
  (nsInlineFrame *) 0x8b216c8 (nsHTMLSpanElement *) 0x88a4358
    (nsContinuingTextFrame *) 0x8b21684 (nsTextNode *) 0x8b15d38
  (nsInlineFrame *) 0x8b21774 (nsHTMLSpanElement *) 0x88a4358
    (nsContinuingTextFrame *) 0x8b21730 (nsTextNode *) 0x8b15d38
  (nsInlineFrame *) 0x8b21820  (nsHTMLSpanElement *) 0x88a4358
    (nsContinuingTextFrame *) 0x8b217dc (nsTextNode *) 0x8b15d38
    (nsInlineFrame *) 0x8b2195c (nsHTMLSpanElement *) 0x8b26f98
    (nsTextFrame *) 0x8b21994 (nsTextNode *) 0x8b29e08

Here the nsAreaFrame is the frame for the XUL element, all the nsInlineFrames that are kids of it are frames for the anonymous span the XBL binding inserts, the text frames are for the text, and the child nsInlineFrame near the end there is the span our constructor just inserted (the one we're using to wrap the accesskey).

Now we append a textnode to this inner span.  At that point, we need the primary frame for that <span>, we go to look for it, end up with the first textframe there as our primary frame hint (which is correct), and get bitten by bug 334829, because of the non-isomorphism of the frame and content trees due to XBL in this case.  So we don't find the frame for the <span>, and never create a frame for the text, with causes this bug.

The patch is twofold.  The frame constructor changes are just the patch for bug 334829, which I think we should just fix on branch.  It's been baking on trunk for a long time, is conceptually the right thing to do, and is very safe, in my opinion.  The presshell changes are a backport of part of the fix for bug 377119 and preserve the "constructors fire before reflow" ordering we used to have in InitialLayout.

The presshell part of the patch is a little riskier (though again I think it's very much sound), but I think we need it to prevent other things that might depend on the ordering there from biting us.

Either change fixes this particular bug, but I do think we should take both on branch.

If we want to be a tiny bit safer, we could drop the restyle processing from the presshell diff; roc, what do you think?
Comment 6 Boris Zbarsky [:bz] 2007-10-07 18:15:06 PDT
Both those bugs have tests in the tree, so no need for separate test here, I think.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-10-07 18:50:02 PDT
Comment on attachment 283948 [details] [diff] [review]

let's do this
Comment 8 Daniel Veditz [:dveditz] 2007-10-08 10:52:56 PDT
Comment on attachment 283948 [details] [diff] [review]

approved for, a=dveditz for release-drivers
Comment 9 Daniel Veditz [:dveditz] 2007-10-08 11:12:26 PDT
Comment on attachment 283948 [details] [diff] [review]

This needs to land on MOZILLA_1_8_BRANCH for future releases AND the GECKO181_20071004_RELBRANCH for the release.

If it helps I can do the merge over to the relbranch for you. Please  ping me if you'd like me to.
Comment 10 Boris Zbarsky [:bz] 2007-10-08 14:46:16 PDT
Checked in on both those branches.
Comment 11 Carsten Book [:Tomcat] 2007-10-09 05:20:01 PDT
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2007100816 Firefox/ and Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv: Gecko/2007100816 Firefox/ (both are the Firefox RC 2 Builds).

The Keyboard Accelerators are now back on 1.8.

Adding verified keyword
Comment 12 Carsten Book [:Tomcat] 2007-11-12 18:12:57 PST
verified fixed using the steps to reproduce from steve and using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2007111203 BonEcho/

The Keyboard Accelerators are okay, adding verified keyword
Comment 13 Boris Zbarsky [:bz] 2007-11-27 10:34:56 PST
This is fixed, since it's a branch-only bug and landed on branches.

Note You need to log in before you can comment on or make changes to this bug.