The default bug view has changed. See this FAQ.

xpfe "history-dropmarker" binding makes invalid assumption about menupop children

RESOLVED FIXED in seamonkey2.1b2

Status

SeaMonkey
Location Bar
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Callek, Assigned: Philip Chee)

Tracking

({fixed-seamonkey2.0.12})

Trunk
seamonkey2.1b2
x86
Windows XP
fixed-seamonkey2.0.12

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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)
(Reporter)

Comment 1

10 years ago
Created attachment 291046 [details] [diff] [review]
check for id
Attachment #291046 - Flags: superreview?(neil)
Attachment #291046 - Flags: review?(neil)

Comment 2

10 years ago
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.
Attachment #291046 - Flags: superreview?(neil)
Attachment #291046 - Flags: review?(neil)
Attachment #291046 - Flags: review-
(Assignee)

Comment 3

10 years ago
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>
(Reporter)

Comment 4

9 years ago
Given Bug 240393 has been fixed, I know of no current problems this would fix.
Severity: normal → trivial
(Reporter)

Updated

9 years ago
Assignee: bugspam.Callek → nobody
QA Contact: guifeatures

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design

Comment 5

8 years ago
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

7 years ago
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?
Target Milestone: seamonkey2.0a1 → ---
(Assignee)

Comment 7

7 years ago
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.
(Reporter)

Comment 8

7 years ago
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
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX

Comment 9

6 years ago
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.
Severity: trivial → normal
Status: RESOLVED → REOPENED
Component: UI Design → Location Bar
QA Contact: ui-design → location-bar
Resolution: WONTFIX → ---
(In reply to comment #9)
> apparently I can't fix this on my end.
You could overlay your <menupopup> somewhere else, such as mainPopupSet.
(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

6 years ago
(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.
(Assignee)

Updated

6 years ago
Status: REOPENED → NEW
(Assignee)

Comment 13

6 years ago
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.
Assignee: nobody → philip.chee
Attachment #291046 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #504093 - Flags: review?(neil)
(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.
(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 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

6 years ago
(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?
(Assignee)

Comment 18

6 years ago
> 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.
(Assignee)

Comment 19

6 years ago
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.
Attachment #504093 - Attachment is obsolete: true
Attachment #504253 - Flags: review?(neil)
Attachment #504093 - Flags: review?(neil)

Updated

6 years ago
Attachment #504253 - Flags: review?(neil) → review+
(Assignee)

Comment 20

6 years ago
Pushed to mozilla-central a=NPOTB DONTBUILD
http://hg.mozilla.org/mozilla-central/rev/daeb7af500a4
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → FIXED
(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.
(Assignee)

Comment 22

6 years ago
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.
Attachment #504954 - Flags: review+
Attachment #504954 - Flags: approval-seamonkey2.0.12?
(Reporter)

Comment 23

6 years ago
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
Attachment #504954 - Flags: approval-seamonkey2.0.12? → approval-seamonkey2.0.12+
(Assignee)

Comment 24

6 years ago
Philor pushed to mozilla-191
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b7a606394564
(Reporter)

Updated

6 years ago
Keywords: fixed-seamonkey2.0.12
(Assignee)

Updated

6 years ago
Target Milestone: --- → seamonkey2.1b2
You need to log in before you can comment on or make changes to this bug.