Last Comment Bug 406396 - xpfe "history-dropmarker" binding makes invalid assumption about menupop children
: xpfe "history-dropmarker" binding makes invalid assumption about menupop chil...
Status: RESOLVED FIXED
: fixed-seamonkey2.0.12
Product: SeaMonkey
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: seamonkey2.1b2
Assigned To: Philip Chee
:
Mentors:
Depends on:
Blocks: 240393
  Show dependency treegraph
 
Reported: 2007-12-01 21:55 PST by Justin Wood (:Callek)
Modified: 2011-01-28 02:42 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
check for id (1.20 KB, patch)
2007-12-01 21:56 PST, Justin Wood (:Callek)
neil: review-
Details | Diff | Review
Patch v1.0 Proposed fix (1.51 KB, patch)
2011-01-15 02:33 PST, Philip Chee
no flags Details | Diff | Review
Patch v1.1 Fix strict errors. (1.28 KB, patch)
2011-01-16 07:57 PST, Philip Chee
neil: review+
Details | Diff | Review
Branch Patch Bv1.0 r=Neil (1.29 KB, patch)
2011-01-18 18:56 PST, Philip Chee
philip.chee: review+
bugspam.Callek: approval‑seamonkey2.0.12+
Details | Diff | Review

Description Justin Wood (:Callek) 2007-12-01 21:55:08 PST
I discovered this on my work for Bug 240393.

The xpfe autocomplete for the url-bar grabs the first menupopup it finds and attempts to show it, we do not want the feeds code to get initialized when there are no feeds in the page, so we need to check for a valid id here.

We are the only consumer of this binding (as far as I can tell)
Comment 1 Justin Wood (:Callek) 2007-12-01 21:56:37 PST
Created attachment 291046 [details] [diff] [review]
check for id
Comment 2 neil@parkwaycc.co.uk 2007-12-02 02:32:40 PST
Comment on attachment 291046 [details] [diff] [review]
check for id

Checking for id in XBL is almost always wrong, unless that id is an attribute on the binding. However setting an id attribute seems suboptimal given that the menupopup is known to be a child of the textbox.

A number of other approaches come to mind. You could search for something else common to all the popups. You could search for the menupopup in a different way. Or you could change the history popup's tag name to popup so that it doesn't conflict with your feed button.

We have four consumers; EdDialogOverlay.xul, MsgAttachPage.xul and openLocation.xul are the other three.
Comment 3 Philip Chee 2007-12-02 04:44:04 PST
FYI: Firefox does this:

      <field name="popup"><![CDATA[
        var popup = null;
        var popupId = this.getAttribute("autocompletepopup");
        if (popupId)
          popup = document.getElementById(popupId);
        if (!popup) {
          popup = document.createElement("popup");
          popup.setAttribute("type", "autocomplete");
          popup.setAttribute("hidden", "true");
          
          var popupset = document.getAnonymousElementByAttribute(this, "anonid", "popupset");
          popupset.appendChild(popup);
        }
        popup.mInput = this;
        popup;
      ]]></field>
Comment 4 Justin Wood (:Callek) 2008-02-16 12:36:26 PST
Given Bug 240393 has been fixed, I know of no current problems this would fix.
Comment 5 Robert Kaiser (not working on stability any more) 2009-08-30 06:24:20 PDT
This bug is open but targeted for seamonkey2.0a1, which has been released a long time ago. Please set the target milestone to an appropriate value, "---" if it has no specific target.
Comment 6 Robert Kaiser (not working on stability any more) 2009-12-20 07:37:59 PST
Is this still relevant? The reporter says in comment #4 that it won't fix any issues, so I wonder if we can just resolve this as WONTFIX or INVALID or so.

Neil? Callek? Ratty?
Comment 7 Philip Chee 2009-12-20 08:09:14 PST
An extension author we encouraged to make his Firefox extension compatible with SeaMonkey ran into another problem with our URLbar due to a similar slight incompatibility> I think it would be worth it to fix this to lower the bar for Firefox extension authors.
Comment 8 Justin Wood (:Callek) 2010-03-25 21:32:17 PDT
as I understand it, Neil is against this atm... We may need to revisit in the future for extension compat reasons as Ratty says in c#7
Comment 9 Dave Garrett 2010-12-31 13:58:16 PST
My thanks to Philip for pointing me here. This bug is apparently the cause of this Flagfox issue under SeaMonkey:
http://flagfox.servehttp.com/forum/viewtopic.php?f=3&t=228

Clicking the arrow for the dropdown for the location bar shows Flagfox's popup instead of its own. Autocomplete does still work fine though. Only a handful of people have ever reported this issue, however, it's still a dumb bug apparently caused by not fixing an obviously bad assumption, which is odd. I have ~1.4 million Firefox users and only 1100 or so SeaMonkey users, but nonetheless would like things to work correctly for them too, and apparently I can't fix this on my end.

Reopening and asking that someone please look into this. Also moving to the location bar component as its description says it covers this and it isn't really a UI design issue.

I did a quick retest and this affects both the current stable release and Trunk.
Comment 10 neil@parkwaycc.co.uk 2010-12-31 14:17:17 PST
(In reply to comment #9)
> apparently I can't fix this on my end.
You could overlay your <menupopup> somewhere else, such as mainPopupSet.
Comment 11 neil@parkwaycc.co.uk 2010-12-31 14:29:11 PST
(In reply to comment #2)
> A number of other approaches come to mind. You could search for something else
> common to all the popups. You could search for the menupopup in a different
> way. Or you could change the history popup's tag name to popup so that it
> doesn't conflict with your feed button.
Well we can't change the tag name any more, since popup no longer works.

One other possibility is to add some XBL code so that the history popup finds the textbox rather than the other way around.
Comment 12 Dave Garrett 2010-12-31 14:29:57 PST
(In reply to comment #10)
> You could overlay your <menupopup> somewhere else, such as mainPopupSet.

I'm not going to put in a hack just for SeaMonkey. The context menu popup for the icon belongs with the icon. It's clearly not part of the main popup set.

Also per comment 7, any other extension up here can be affected by this.
Comment 13 Philip Chee 2011-01-15 02:33:32 PST
Created attachment 504093 [details] [diff] [review]
Patch v1.0 Proposed fix

> We have four consumers; EdDialogOverlay.xul, MsgAttachPage.xul and

> openLocation.xul are the other three.

Tested in all four. This patch works in all except EdDialogOverlay.xul. But then autocomplete dropdown didn't work even without this patch.

Tested with flagfox-4.1a4-fx+sm.xpi and the right autocomplete drop down appears.

> +          var popup = textbox.getElementsByClassName("autocomplete-history-popup")[0];

I thought that getElementsByClassName() would work only for HTML DOMs but I tried this and it worked. I guess Gecko 2.0 added this to the XUL DOM.
Comment 14 neil@parkwaycc.co.uk 2011-01-15 04:53:44 PST
(In reply to comment #13)
> Tested in all four. This patch works in all except EdDialogOverlay.xul. But
> then autocomplete dropdown didn't work even without this patch.
The entire dialog seems to be broken when editing an existing link :-( But if you have an existing heading or anchor then it should appear in the dropdown.
Comment 15 neil@parkwaycc.co.uk 2011-01-15 04:54:33 PST
(In reply to comment #12)
> (In reply to comment #10)
> > You could overlay your <menupopup> somewhere else, such as mainPopupSet.
> I'm not going to put in a hack just for SeaMonkey.
It wasn't clear what you meant by "I can't fix this"
Comment 16 neil@parkwaycc.co.uk 2011-01-15 04:56:04 PST
Comment on attachment 504093 [details] [diff] [review]
Patch v1.0 Proposed fix

>+          var popup = textbox.getElementsByClassName("autocomplete-history-popup")[0];
This is a JS strict warning if there is no popup, which is why the old code fiddled around with .item(0) instead.
Comment 17 Dave Garrett 2011-01-15 07:18:43 PST
(In reply to comment #15)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > You could overlay your <menupopup> somewhere else, such as mainPopupSet.
> > I'm not going to put in a hack just for SeaMonkey.
> It wasn't clear what you meant by "I can't fix this"

Yeah, of course it's probably possible to hack around a bunch of bugs. In fact, I have an ugly hack in place to deal with bug 509800 but that doesn't mean I don't want it fixed. I've been removing special cases and hacks for specific browsers and Gecko 1.9.0 for the next version and would like to continue in that direction rather than having to add more workarounds and hacks. ;)

(In reply to comment #13)
> Tested in all four. This patch works in all except EdDialogOverlay.xul. But
> then autocomplete dropdown didn't work even without this patch.
> 
> Tested with flagfox-4.1a4-fx+sm.xpi and the right autocomplete drop down
> appears.

Great!

> > +          var popup = textbox.getElementsByClassName("autocomplete-history-popup")[0];
> 
> I thought that getElementsByClassName() would work only for HTML DOMs but I
> tried this and it worked. I guess Gecko 2.0 added this to the XUL DOM.

So then there's no equivalent quick fix that could be backported to SeaMonkey 2.0? It would be nice if possible, but it's not a show stopper. Is getElementById not usable here for some reason?
Comment 18 Philip Chee 2011-01-15 07:48:04 PST
> So then there's no equivalent quick fix that could be backported to SeaMonkey

I just checked and yes getElementsByClassName() does work in XUL. I suppose I was mistaken.

> 2.0? It would be nice if possible, but it's not a show stopper. Is
> getElementById not usable here for some reason?

See comment 2.
Comment 19 Philip Chee 2011-01-16 07:57:05 PST
Created attachment 504253 [details] [diff] [review]
Patch v1.1 Fix strict errors.

> neil@parkwaycc.co.uk      2011-01-15 04:56:04 PST
> 
>>+          var popup = textbox.getElementsByClassName("autocomplete-history-popup")[0];
> This is a JS strict warning if there is no popup, which is why the old code
> fiddled around with .item(0) instead.
Fixed.
Comment 20 Philip Chee 2011-01-16 20:46:55 PST
Pushed to mozilla-central a=NPOTB DONTBUILD
http://hg.mozilla.org/mozilla-central/rev/daeb7af500a4
Comment 21 :Ms2ger 2011-01-17 05:54:37 PST
(In reply to comment #17)
> > > +          var popup = textbox.getElementsByClassName("autocomplete-history-popup")[0];
> > 
> > I thought that getElementsByClassName() would work only for HTML DOMs but I
> > tried this and it worked. I guess Gecko 2.0 added this to the XUL DOM.
> 
> So then there's no equivalent quick fix that could be backported to SeaMonkey
> 2.0? It would be nice if possible, but it's not a show stopper. Is
> getElementById not usable here for some reason?

This has always been supported; see bug 357450.
Comment 22 Philip Chee 2011-01-18 18:56:24 PST
Created attachment 504954 [details] [diff] [review]
Branch Patch Bv1.0 r=Neil

Requesting SeaMonkey2.0.12 branch approval.

Performance impact: AFAIK none.

Risk assessment: Since this is a correctness one-line fix risk is low to none.
Gain: Fixes a problem we cause for at least one extension (Flagfox) and removes the need for extensions to do hacky workarounds to avoid this bug.
Comment 23 Justin Wood (:Callek) 2011-01-18 19:30:53 PST
Comment on attachment 504954 [details] [diff] [review]
Branch Patch Bv1.0 r=Neil

If you can get this landed in 24 hours, a2.0.12=me if not, please wait until after I kick our build1's for 2.0.12 and this can land after that for 2.0.13
Comment 24 Philip Chee 2011-01-18 20:25:23 PST
Philor pushed to mozilla-191
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b7a606394564

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