1235154 - TypeError this.parentNode.firstChild.focus is not a function

Status: RESOLVED FIXED

(SeaMonkey :: Location Bar, defect)

Description

[Frank-Rainer Grahl (:frg)]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0 SeaMonkey/2.41
Build ID: 20151224203712

Steps to reproduce:

Right click in the url field reports the following error starting with Seamonkey 2.41.


Actual results:

Error in weblog is reported:

Timestamp: 25.12.2015 14:29:08
Error: TypeError: this.parentNode.firstChild.focus is not a function
Source File: chrome://navigator/content/navigator.xul
Line: 1


Expected results:

No error should occur.

User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0 SeaMonkey/2.41
Build identifier: 20151224203712

Comment 1

[Frank-Rainer Grahl (:frg)]

It's not limited to x86_64. I can reproduce it under Vista and all trees but release. Fresh profile.

User agent: Mozilla/5.0 (Windows NT 6.0; rv:45.0) Gecko/20100101 Firefox/45.0 SeaMonkey/2.42a2
Build identifier: 20151223140842

Comment 2

[Tony Mechelynck [:tonymec]]

(In reply to Frank-Rainer Grahl from comment #1)
> It's not limited to x86_64. I can reproduce it under Vista and all trees but
> release. Fresh profile.
> 
> User agent: Mozilla/5.0 (Windows NT 6.0; rv:45.0) Gecko/20100101
> Firefox/45.0 SeaMonkey/2.42a2
> Build identifier: 20151223140842

"Release" is still 2.39 IIUC; yet I suppose "beta" is the 2.41 you mentioned in comment #0. 2.42a2 Aurora is mentioned in comment #1 by its UA and build ID; and AFAIK no 2.43a1 trunk build was ever published for Windows, so I'll not yet decide if this bug is a "Trunk" bug.

I tried to reproduce the bug on 2.43a1 for Linux64, but couldn't:

UA:"Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0 SeaMonkey/2.43a1" ID:20151217003002 c-c:c7fb0822b04746fcb1321ab430a471160459e20c m-c:0babaa3edcf908c393b68a3dc2d1c2a2450c31ed en-US

Comment 3

[Frank-Rainer Grahl (:frg)]

Sorry for being unclear. The problem can be seen in all my private Windows x86 and x64 VS2013 and VS2015 builds from the current comm-beta and up trees. I am not seeing this in a 2.40 built from the current comm-release tree. It's pretty easy to confirm it on Windows.

I just tried another unofficial build:

seamonkey-2.41.en-US.win32.installer.exe from

https://l10n.mozilla-community.org/~akalla/unofficial/seamonkey/nightly/latest-comm-beta-windows32/

User agent: Mozilla/5.0 (Windows NT 6.1; rv:43.0) Gecko/20100101 Firefox/43.0 SeaMonkey/2.41
Build identifier: 20151219183444

also shows it. Seems to be mostly cosmetic. The popup menu works.

FRG

Comment 4

[Tony Mechelynck [:tonymec]]

(In reply to Frank-Rainer Grahl from comment #3)
> Sorry for being unclear. The problem can be seen in all my private Windows
> x86 and x64 VS2013 and VS2015 builds from the current comm-beta and up
> trees.

Does that include 2.43a1, which AFAIK the official builders have not succeeded to build yet? No need to compile one yourself, you can get one from one of the following sources ("unofficial", but built from the official sources AFAIK):
https://l10n.mozilla-community.org/~akalla/unofficial/seamonkey/nightly/latest-comm-central-windows32/
https://l10n.mozilla-community.org/~akalla/unofficial/seamonkey/nightly/latest-comm-central-windows64/
What you find there is a full en-US build, plus a number of .langpack.xpi add-ons: if, for instance, you want Gerpman menus and messages, install the full build plus the .de.langpack.xpi, then select "German" in "Edit → Preferences → Appearance → User Interface Language" if it doesn't start up in German.
These builds' symbols aren't known by Mozilla's Socorro server, but if you know how to use the *.crashreporters-symbols.zip to translate addresses into symbols, you should download it in addition to the *.txt and *.installer.exe (or .complete.mar if you know how to use that; I don't).

Comment 5

[Frank-Rainer Grahl (:frg)]

Yes private 2.43a1 is also affected. These were all en-us builds. I usually build all trees now just for the fun of it:) 

User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0 SeaMonkey/2.43a1
Build identifier: 20151224200401

Comment 6

[Tony Mechelynck [:tonymec]]

Can someone reproduce this bug? Philip, you're on Windows aren't you?

Downgrading to "minor" because reporter said in comment #3 that "the problem seems to be mostly cosmetic".

Comment 7

[therube]

Confirmed - using the 2.41 build from ~akalla.

Comment 8

[Philip Chee]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from Bug 869086 comment #0)

> One of the changes coming in bug 653881 is that the XBL code will no longer
> remove <children> elements from the XBL binding's DOM tree. For nested
> bindings that touch the binding's DOM, this causes elements to move around.
> Currently textbox.xml simply uses this.parentNode.firstChild and breaks with
> the change, but is also fragile in the face of any manual changes.

> This patch makes things work in both the new and old worlds.

Looks like we need to port the changes in http://hg.mozilla.org/mozilla-central/rev/775ab8588cec

This goes back to mozilla25. I don't know why it only bit us in 2.41

Comment 9

[Frank-Rainer Grahl (:frg)]

Seems to be a simple patch in suite/browser/urlbarBindings.xml.

Compiling c-c right now. If it works I will upload it soon. Not that I know 100% what it does:)

FRG

Comment 10

[Frank-Rainer Grahl (:frg)]

Attachment: 1235154.patch

Patch works. Error goes away I didn't notice before but every item in the popup menu is now conditionally enabled or disabled as it should be. Who can review this?

Comment 11

[Tony Mechelynck [:tonymec]]

Comment on attachment 8702077 [details] [diff] [review]
1235154.patch

(In reply to Frank-Rainer Grahl from comment #10)
> Patch works. Error goes away I didn't notice before but every item in the
> popup menu is now conditionally enabled or disabled as it should be. Who can
> review this?

Neil certainly could, but I guess he's overworked. Philip, do you think it's in your line? Otherwise, please nominate someone else.

Comment 12

[Philip Chee]

Comment on attachment 8702077 [details] [diff] [review]
1235154.patch

As I was reminded recently, the commit comment should reflect what the patch does rather than what the bug summary says. Perhaps something like this?
> Bug 1235154 - Port Bug 869086 to fix right click urlbar error [TypeError this.parentNode.firstChild.focus is not a function]

Plus maybe this:
> References:
> Bug 869086 - Make textbox.xml resilient against the changes in bug 653881
> Bug 653881 - Fix up our XBL1 shadow tree implementation so that we can reuse it for XBL2

Comment 13

[Frank-Rainer Grahl (:frg)]

Thanks. I will keep this in mind. Just copied from some other patches I saw. I always check the bug if I need more info so I didn't think I would need more comments anyway.

2.40, no wonder, is also affected even if no error is shown. All popup items are always enabled. The patch applies clean to all trees. Just updated my local c-c, c-a, c-b and c-r repositories.

Comment 14

[Tony Mechelynck [:tonymec]]

After you modify the patch header according to comment #12 you can carryover the r+ and set the checkin-needed keyword.

If you used the Mercurial mq extension to produce the patch, you can do (after qpush'ing it if necessary) "hg qrefresh -e" to edit the commit message by means of your favourite editor. If you don't have vim (the hg default) or if you want another _pure-text_ editor (Notepad maybe?) set the editor item in the [ui] section of your .hgrc or Mercurial.ini as a first step.

Comment 15

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8702077 [details] [diff] [review]
1235154.patch

Bug 1235154 - Port Bug 869086 to fix right click urlbar error [TypeError this.parentNode.firstChild.focus is not a function]

References:
Bug 869086 - Make textbox.xml resilient against the changes in bug 653881
Bug 653881 - Fix up our XBL1 shadow tree implementation so that we can reuse it for XBL2

Comment 16

[Frank-Rainer Grahl (:frg)]

Hmm thought comment would maybe insert it but not so. If I upload a new patch and obsolete the old one wouldn't the review need to be renewed?

Comment 17

[Philip Chee]

(In reply to Frank-Rainer Grahl from comment #16)
> Hmm thought comment would maybe insert it but not so. If I upload a new
> patch and obsolete the old one wouldn't the review need to be renewed?
You can set the review flag to + yourself (if you have editbugs) but it would be nice if you added "carrying forward r+ from poohbah" to the bug description textbox to make things clear.

Comment 18

[Tony Mechelynck [:tonymec]]

(In reply to Philip Chee from comment #17)
> (In reply to Frank-Rainer Grahl from comment #16)
> > Hmm thought comment would maybe insert it but not so. If I upload a new
> > patch and obsolete the old one wouldn't the review need to be renewed?
> You can set the review flag to + yourself (if you have editbugs) but it
> would be nice if you added "carrying forward r+ from poohbah" to the bug
> description textbox to make things clear.

Yes you need to upload a new version of the patch, obsolete the old one, set r+ with a comment saying "carrying forward r+ from Philip Chee" or similar, and since you're the reporter you can do it even without editbugs. Then after checking that the new version of the patch has correct headers you may set the checkin-needed keyword on the bug and someone will push the patch and report the corresponding changeset.

Comment 19

[Frank-Rainer Grahl (:frg)]

I asked for editbugs permission and will try when I get them. If I donÄt get them till the end of the week I will try to screw it up without them :)

Thanks for you help.
FRG

Comment 20

[Tony Mechelynck [:tonymec]]

(In reply to Frank-Rainer Grahl from comment #19)
> I asked for editbugs permission and will try when I get them. If I donÄt get
> them till the end of the week I will try to screw it up without them :)
> 
> Thanks for you help.
> FRG

You can still upload the new version of the patch. If you don't see a + item on the "review" rolldown, Philip or I will set it.

Comment 21

[Frank-Rainer Grahl (:frg)]

Would like to wait and try it with the new rights so that I can see what I can do with them. 

Have fun
FRG

Comment 22

[Frank-Rainer Grahl (:frg)]

Attachment: 1235154-V2.patch

Changed comment in header 

carrying forward r+ from Philip Chee

Comment 23

[Frank-Rainer Grahl (:frg)]

Hope I did it right. Philip and Tony: Thanks for your guidance.

FRG

Comment 24

[Tony Mechelynck [:tonymec]]

(In reply to Frank-Rainer Grahl from comment #23)
> Hope I did it right. Philip and Tony: Thanks for your guidance.
> 
> FRG

That new bug heading is great. (I'm not competent to check the body.) And, it's always nice to have a new volunteer (we need them). You may want to add a roundtable entry in your name at https://wiki.mozilla.org/SeaMonkey/StatusMeetings/2016-01-05#Roundtable_-_Personal_Status_Updates and, if you can, participate in the next SeaMonkey Status meeting at irc://moznet/seamonkey (clicking this link in SeaMonkey, or copying to your URL bar, will open it in ChatZilla) on 5 January at 14h Central European Time.

Comment 25

[Philip Chee]

Normally if you don't have the credentials to push you should set the checkin-needed flag but nobody is going to notice that until the new year festivities are over.

Pushed to comm-central http://hg.mozilla.org/comm-central/rev/77197abd9d73

Comment 26

[Frank-Rainer Grahl (:frg)]

>> you should set the checkin-needed flag 

Isn't that the keyword which I did set?  No such flag otherwise that I can see in the patch details or general flags.

FRG

Comment 27

[Philip Chee]

Comment on attachment 8702357 [details] [diff] [review]
1235154-V2.patch

[Approval Request Comment]
Regression caused by (bug #): Bug 653881 [XBL1 shadow tree Fix up]
User impact if declined: urlbar context menu items are all enabled inappropriately
Testing completed (on m-c, etc.): Bug 869086 tested on mozilla23+
Risk to taking this patch (and alternatives if risky): None. Bustage fix.
String changes made by this patch: None.

Comment 28

[Tony Mechelynck [:tonymec]]

(In reply to Frank-Rainer Grahl from comment #26)
> >> you should set the checkin-needed flag 
> 
> Isn't that the keyword which I did set?  No such flag otherwise that I can
> see in the patch details or general flags.
> 
> FRG

Yes, you did, between comment #22 and #23. We're still in the New Year holidays and close to the New Year itself so we're lucky that Philip isn't 100% busy with that (maybe he'll feast the Chinese New Year in about a month) and that he has push permissions. :-)

Comment 29

[Tony Mechelynck [:tonymec]]

@Philip: I wonder why you changed the 2.40 tracking flag from unaffected (see comment #3) to wontfix. Of course, in neither case will the patch be ported to that branch.

Comment 30

[Philip Chee]

(In reply to Tony Mechelynck [:tonymec] from comment #29)
> @Philip: I wonder why you changed the 2.40 tracking flag from unaffected
> (see comment #3) to wontfix. Of course, in neither case will the patch be
> ported to that branch.
Mistake. I blame it on the New Year holidays :P

Comment 31

[Philip Chee]

http://hg.mozilla.org/releases/comm-aurora/rev/d3e81ff2cf54
http://hg.mozilla.org/releases/comm-beta/rev/bd946322edbd