398837 - [FIX]Keyboard accelerators missing on Security Warning dialog

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Steve England [:stevee]]

Attachment: Note the missing letters where the keyboard accelerators should be

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

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

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

Actual:
- 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:1.8.1.7) Gecko/2007091417 Firefox/2.0.0.7 with a new profile.

Found by Warduke at http://forums.mozillazine.org/viewtopic.php?p=3085777#3085777

Comment 1

[Steve England [:stevee]]

works: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.8pre) Gecko/2007093004 BonEcho/2.0.0.8pre

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

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

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=AviarySuiteBranchTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-09-30+03&maxdate=2007-10-01+04&cvsroot=%2Fcvsroot

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]]

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?
http://lxr.mozilla.org/mozilla1.8/source/browser/components/preferences/securityWarnings.xul

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: [e-mail]]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Attachment: Fix

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 [:bzbarsky]]

Both those bugs have tests in the tree, so no need for separate test here, I think.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 283948 [details] [diff] [review]
Fix

let's do this

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 283948 [details] [diff] [review]
Fix

approved for 1.8.1.8, a=dveditz for release-drivers

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 283948 [details] [diff] [review]
Fix

This needs to land on MOZILLA_1_8_BRANCH for future releases AND the GECKO181_20071004_RELBRANCH for the 1.8.1.8 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 [:bzbarsky]]

Checked in on both those branches.

Comment 11

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.8 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8) Gecko/2007100816 Firefox/2.0.0.8 and Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.8) Gecko/2007100816 Firefox/2.0.0.8 (both are the Firefox 2.0.0.8 RC 2 Builds).

The Keyboard Accelerators are now back on 1.8.

Adding verified keyword

Comment 12

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.10 using the steps to reproduce from steve and using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.10pre) Gecko/2007111203 BonEcho/2.0.0.10pre

The Keyboard Accelerators are okay, adding verified keyword

Comment 13

[Boris Zbarsky [:bzbarsky]]

This is fixed, since it's a branch-only bug and landed on branches.