[FIX]Keyboard accelerators missing on Security Warning dialog




10 years ago
10 years ago


(Reporter: stevee, Assigned: bz)


({regression, verified1.8.1.10, verified1.8.1.8})

1.8 Branch
regression, verified1.8.1.10, verified1.8.1.8
Bug Flags:
blocking1.8.1.8 +
blocking1.8.1.10 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(2 attachments)



10 years ago
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 http://forums.mozillazine.org/viewtopic.php?p=3085777#3085777
Flags: blocking1.8.1.8?

Comment 1

10 years ago
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.
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.

Assignee: nobody → bzbarsky
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).
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.


10 years ago
Assignee: bzbarsky → nobody
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Version: 2.0 Branch → 1.8 Branch


10 years ago
Assignee: nobody → bzbarsky
Blocks: 267833
OS: Windows 2000 → All
Hardware: PC → All
Summary: Keyboard accelerators missing on Security Warning dialog → [FIx]Keyboard accelerators missing on Security Warning dialog


10 years ago
Summary: [FIx]Keyboard accelerators missing on Security Warning dialog → [FIX]Keyboard accelerators missing on Security Warning dialog
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?
Attachment #283948 - Flags: superreview?(roc)
Attachment #283948 - Flags: review?(roc)
Attachment #283948 - Flags: approval1.8.1.8?
Both those bugs have tests in the tree, so no need for separate test here, I think.
Flags: in-testsuite-
Comment on attachment 283948 [details] [diff] [review]

let's do this
Attachment #283948 - Flags: superreview?(roc)
Attachment #283948 - Flags: superreview+
Attachment #283948 - Flags: review?(roc)
Attachment #283948 - Flags: review+
Comment on attachment 283948 [details] [diff] [review]

approved for, a=dveditz for release-drivers
Attachment #283948 - Flags: approval1.8.1.8? → approval1.8.1.8+
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.
Flags: blocking1.8.1.9+
Flags: blocking1.8.1.8?
Flags: blocking1.8.1.8+
Checked in on both those branches.
Flags: blocking1.8.1.8+ → blocking1.8.1.8?
Keywords: fixed1.8.1.8, fixed1.8.1.9
Flags: blocking1.8.1.8? → blocking1.8.1.8+
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
Keywords: fixed1.8.1.8 → verified1.8.1.8
Flags: blocking1.8.1.11+ → blocking1.8.1.10+
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
Keywords: fixed1.8.1.10 → verified1.8.1.10
This is fixed, since it's a branch-only bug and landed on branches.
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.