Closed Bug 642420 Opened 9 years ago Closed 8 years ago

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

Categories

(Toolkit :: XUL Widgets, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla6
Tracking Status
firefox6 --- fixed

People

(Reporter: sgautherie, Assigned: sgautherie)

References

()

Details

(Whiteboard: [fixed in moz5: Av1; moz6: Bv1])

Attachments

(2 files)

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>
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 on attachment 519908 [details] [diff] [review]
(Av1) Just remove it, Update a11y test_combobox.xul
[Checked in: Comment 6]

Needs a11y review too right?
Attachment #519908 - Flags: review?(neil) → review+
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).
Attachment #519908 - Flags: feedback?(surkov.alexander)
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.
Attachment #519908 - Flags: feedback?(surkov.alexander) → feedback+
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
Attachment #519908 - Attachment description: (Av1) Just remove it, Update a11y test_combobox.xul → (Av1) Just remove it, Update a11y test_combobox.xul [Checked in: Comment 6]
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Summary: XPFE autocomplete.xml: investigate removing explicit <children includes="menupopup"/>. → XPFE autocomplete.xml: investigate removing explicit <children includes="menupopup"/>
Target Milestone: --- → mozilla2.0
Target Milestone: mozilla2.0 → mozilla2.2
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'.
Attachment #519908 - Flags: approval2.0?
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
(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.)
Ping for approval.
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
Attachment #519908 - Flags: approval2.0? → approval2.0-
This caused bug 665538.
Status: RESOLVED → REOPENED
Depends on: 665538
Resolution: FIXED → ---
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)?
(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.
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.
Attachment #543709 - Flags: review?(neil)
Attachment #543709 - Flags: approval-mozilla-beta?
Attachment #543709 - Flags: approval-mozilla-aurora?
I've applied the patch to a SM 2.2b3 Win32 build and it fixes bug 665538 for me.
Attachment #543709 - Flags: review?(neil) → review+
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
Attachment #543709 - Attachment description: (Bv1) XPFE autocomplete.xml: restore explicit <children includes="menupopup"/>, but at start instead of at end → (Bv1) XPFE autocomplete.xml: restore explicit <children includes="menupopup"/>, but at start instead of at end [Checked in: Comment 17]
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [SeaMonkey v2.2-v2.3 are not unregressed (yet)]
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.
Or better still, mark NPOMCB patches with DONTBUILD in the checkin comment.
Attachment #543709 - Flags: approval-mozilla-beta?
Attachment #543709 - Flags: approval-mozilla-beta+
Attachment #543709 - Flags: approval-mozilla-aurora?
Attachment #543709 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
Whiteboard: [SeaMonkey v2.2-v2.3 are not unregressed (yet)] → [c-n: Bv1 to m-a & m-b] [SeaMonkey v2.2-v2.3 are not unregressed (yet)]
Keywords: checkin-needed
Whiteboard: [c-n: Bv1 to m-a & m-b] [SeaMonkey v2.2-v2.3 are not unregressed (yet)]
Attachment #543709 - Attachment description: (Bv1) XPFE autocomplete.xml: restore explicit <children includes="menupopup"/>, but at start instead of at end [Checked in: Comment 17] → (Bv1) XPFE autocomplete.xml: restore explicit <children includes="menupopup"/>, but at start instead of at end [Checked in: Comment 17 & 20]
(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 :-<
Whiteboard: [fixed in moz5: Av1; moz6: Bv1]
Target Milestone: mozilla5 → mozilla6
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.
Pushed by frgrahl@gmx.net:
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.
You need to log in before you can comment on or make changes to this bug.