Last Comment Bug 642420 - XPFE autocomplete.xml: investigate removing explicit <children includes="menupopup"/>
: XPFE autocomplete.xml: investigate removing explicit <children includes="menu...
Status: RESOLVED FIXED
[fixed in moz5: Av1; moz6: Bv1]
:
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla6
Assigned To: Serge Gautherie (:sgautherie)
:
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on: 535893 665538
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-17 05:28 PDT by Serge Gautherie (:sgautherie)
Modified: 2012-01-15 19:35 PST (History)
5 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
(Av1) Just remove it, Update a11y test_combobox.xul [Checked in: Comment 6] (2.51 KB, patch)
2011-03-17 07:36 PDT, Serge Gautherie (:sgautherie)
neil: review+
surkov.alexander: feedback+
dveditz: approval2.0-
Details | Diff | Splinter Review
(Bv1) XPFE autocomplete.xml: restore explicit <children includes="menupopup"/>, but at start instead of at end [Checked in: Comment 17 & 20] (1.23 KB, patch)
2011-07-03 19:52 PDT, Serge Gautherie (:sgautherie)
neil: review+
asa: approval‑mozilla‑aurora+
asa: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2011-03-17 05:28:14 PDT
The goal would be to have the XPFE children in the same order as the Toolkit children, to reduce differences.

Bug 535893 comment 11:
{
neil@parkwaycc.co.uk      2011-03-17 03:27:35 PDT

The XPFE autocomplete has an extra explicit <children includes="menupopup"/>
entry in its XBL but toolkit seems to manage perfectly well without it. Maybe
worth a followup bug to investigate removing it.
}

Code is:
9   <binding id="autocomplete"
16     <content sizetopopup="pref">
40       <children includes="menupopup"/>
41     </content>
Comment 1 Serge Gautherie (:sgautherie) 2011-03-17 07:32:16 PDT
Ftr,

This line was added in XPFE in:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpfe/components/autocomplete/resources/content/autocomplete.xml&rev=1.33
1.33	hewitt%netscape.com	2001-04-30 11:36	 	43189 - autocomplete, r=matt, sr=alecf

It looks like Toolkit never had this line:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/toolkit/content/widgets/autocomplete.xml&rev=1.1
1.1	hewitt%netscape.com	2002-09-27 23:21	 	165955 - turning on satchel in phoenix
Comment 2 Serge Gautherie (:sgautherie) 2011-03-17 07:36:47 PDT
Created attachment 519908 [details] [diff] [review]
(Av1) Just remove it, Update a11y test_combobox.xul
[Checked in: Comment 6]
Comment 3 neil@parkwaycc.co.uk 2011-03-17 15:28:28 PDT
Comment on attachment 519908 [details] [diff] [review]
(Av1) Just remove it, Update a11y test_combobox.xul
[Checked in: Comment 6]

Needs a11y review too right?
Comment 4 Serge Gautherie (:sgautherie) 2011-03-17 16:29:33 PDT
Comment on attachment 519908 [details] [diff] [review]
(Av1) Just remove it, Update a11y test_combobox.xul
[Checked in: Comment 6]

(In reply to comment #3)
> Needs a11y review too right?

I don't think so in this case, but I'll ask for feedback (at least).
Comment 5 alexander :surkov 2011-03-20 19:20:30 PDT
Comment on attachment 519908 [details] [diff] [review]
(Av1) Just remove it, Update a11y test_combobox.xul
[Checked in: Comment 6]

feedback is ok here, but yes - either review or feedback from a11y peer is required when a11y mochitest are changed.
Comment 6 Serge Gautherie (:sgautherie) 2011-03-21 06:50:25 PDT
Comment on attachment 519908 [details] [diff] [review]
(Av1) Just remove it, Update a11y test_combobox.xul
[Checked in: Comment 6]

http://hg.mozilla.org/mozilla-central/rev/ddd48e3b173a
Comment 7 Serge Gautherie (:sgautherie) 2011-03-26 16:13:54 PDT
Comment on attachment 519908 [details] [diff] [review]
(Av1) Just remove it, Update a11y test_combobox.xul
[Checked in: Comment 6]

"approval2.0=?":
NPOTDB, FF2SM sync'.
Comment 8 Daniel Veditz [:dveditz] 2011-04-01 10:48:56 PDT
Comment on attachment 519908 [details] [diff] [review]
(Av1) Just remove it, Update a11y test_combobox.xul
[Checked in: Comment 6]

I didn't understand the rationale in that last comment for why this must land on the mozilla2.0 repository
Comment 9 Serge Gautherie (:sgautherie) 2011-04-01 13:38:28 PDT
(In reply to comment #8)
> I didn't understand the rationale in that last comment for why this must land
> on the mozilla2.0 repository

So upcoming SeaMonkey 2.1 can have this fix. (Firefox is unaffected.)
Comment 10 Serge Gautherie (:sgautherie) 2011-04-30 18:16:02 PDT
Ping for approval.
Comment 11 Daniel Veditz [:dveditz] 2011-05-11 11:03:37 PDT
Comment on attachment 519908 [details] [diff] [review]
(Av1) Just remove it, Update a11y test_combobox.xul
[Checked in: Comment 6]

not more planned 4.0.x releases, minused
Comment 12 neil@parkwaycc.co.uk 2011-06-29 16:01:28 PDT
This caused bug 665538.
Comment 13 Serge Gautherie (:sgautherie) 2011-06-29 17:46:18 PDT
In order to back this out, at least ftb,
Would it be possible to move |<children includes="menupopup"/>| around to still get the same child order as Toolkit has?
What comment could be added to explain a little why that code is needed (wrt bug 665538)?
Comment 14 neil@parkwaycc.co.uk 2011-06-30 01:31:56 PDT
(In reply to comment #13)
> Would it be possible to move |<children includes="menupopup"/>| around to
> still get the same child order as Toolkit has?
Possibly. Try putting it at the start of the binding.
Comment 15 Serge Gautherie (:sgautherie) 2011-07-03 19:52:33 PDT
Created attachment 543709 [details] [diff] [review]
(Bv1) XPFE autocomplete.xml: restore explicit <children includes="menupopup"/>, but at start instead of at end
[Checked in: Comment 17 & 20]

Tested (with an old "2.1.1pre" build, as newer omni.jar can't be opened/edited).

"approval-mozilla-aurora=?" and "approval-mozilla-beta=?":
Unregress SeaMonkey v2.2+ from patch Av1. NPOfirefoxB, no risk.
Comment 16 rsx11m 2011-07-03 20:35:49 PDT
I've applied the patch to a SM 2.2b3 Win32 build and it fixes bug 665538 for me.
Comment 17 Serge Gautherie (:sgautherie) 2011-07-04 05:38:22 PDT
Comment on attachment 543709 [details] [diff] [review]
(Bv1) XPFE autocomplete.xml: restore explicit <children includes="menupopup"/>, but at start instead of at end
[Checked in: Comment 17 & 20]

http://hg.mozilla.org/mozilla-central/rev/29bc151be813
Comment 18 Marco Bonardo [::mak] 2011-07-04 08:34:52 PDT
Please, when pushing to central:
1. Care about your push: star oranges asap, retrigger broken builds.
2. Be available in #developers.

Otherwise, we created the mozilla-inbound repository for whoever doesn't have the time or will to follow his own pushes. So just push in mozilla-inbound and people will take care of doing that for you.

Putting more work on sheriff's shoulders is not going to help anyone.
Thanks.
Comment 19 neil@parkwaycc.co.uk 2011-07-04 08:38:24 PDT
Or better still, mark NPOMCB patches with DONTBUILD in the checkin comment.
Comment 21 Serge Gautherie (:sgautherie) 2011-09-30 22:18:41 PDT
(In reply to Marco Bonardo [:mak] from comment #18)
> Otherwise, we created the mozilla-inbound repository for whoever doesn't
> have the time or will to follow his own pushes.

I wasn't familiar with mozilla-inbound.
I will try to use it, or just use checkin-needed.


(In reply to neil@parkwaycc.co.uk from comment #19)
> Or better still, mark NPOMCB patches with DONTBUILD in the checkin comment.

Right. I did it for patch Av1, but I did miss this for patch Bv1 :-<
Comment 22 Serge Gautherie (:sgautherie) 2012-01-15 19:35:01 PST
Comment on attachment 543709 [details] [diff] [review]
(Bv1) XPFE autocomplete.xml: restore explicit <children includes="menupopup"/>, but at start instead of at end
[Checked in: Comment 17 & 20]

Ftr, this unregression patch never made it to mozilla5.

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