642420 - XPFE autocomplete.xml: investigate removing explicit <children includes="menupopup"/>

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Serge Gautherie (:sgautherie)]

The goal would be to have the XPFE children in the same order as the Toolkit children, to reduce differences.

Bug 535893 comment 11:
{
[e-mail]      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)]

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

Attachment: (Av1) Just remove it, Update a11y test_combobox.xul [Checked in: Comment 6]

Comment 3

[[e-mail]]

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

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 (:asurkov)]

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

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

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

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

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

Ping for approval.

Comment 11

[Daniel Veditz [:dveditz]]

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

[[e-mail]]

This caused bug 665538.

Comment 13

[Serge Gautherie (:sgautherie)]

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

[[e-mail]]

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

Attachment: (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]

I've applied the patch to a SM 2.2b3 Win32 build and it fixes bug 665538 for me.

Comment 17

[Serge Gautherie (:sgautherie)]

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

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

[[e-mail]]

Or better still, mark NPOMCB patches with DONTBUILD in the checkin comment.

Comment 20

[LegNeato]

http://hg.mozilla.org/releases/mozilla-beta/rev/a49dce91dc3c

Comment 21

[Serge Gautherie (:sgautherie)]

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

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.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/acb8957ae568
XPFE autocomplete.xml: investigate removing explicit <children includes="menupopup"/>; (Av1) Just remove it, Update a11y test_combobox.xul.
https://hg.mozilla.org/comm-central/rev/b674202b178a
(Bv1) XPFE autocomplete.xml: restore explicit <children includes="menupopup"/>, but at start instead of at end.