1502997 - Adapt to bug 1502875 - GetElementById no longer finds anonymous elements

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[:aceman]

Bug 1502875 - Don't let GetElementById work across anonymous subtree boundaries in XUL documents. r=smaug

Comment 1

[:aceman]

If TB is relying on document.getElementById traversing into XBL content (anonymous elements), behavior can be restored by using something like `document.getAnonymousElementByAttribute(xblHostElement, "id", id)` (see bug 1502151 and bug 1500240).

Comment 2

[:aceman]

By running mozmill on Linux I found these ids defined on anonymous elements:
Anonymous id=addonsButton
Anonymous id=category-box
Anonymous id=dialogOverlay-0
Anonymous id=dialogStack
Anonymous id=dialogTemplate
Anonymous id=largeTooltip
Anonymous id=mailbox://nobody@Local%20Folders
Anonymous id=mailbox://nobody@Local%20Folders/ColumnsA
Anonymous id=mailbox://nobody@Local%20Folders/ColumnsApplyParent
Anonymous id=mailbox://nobody@Local%20Folders/ColumnsApplyParent/Child1
Anonymous id=mailbox://nobody@Local%20Folders/ColumnsApplyParent/Child2
Anonymous id=mailbox://nobody@Local%20Folders/ColumnsApplySource
Anonymous id=mailbox://nobody@Local%20Folders/ColumnsB
Anonymous id=mailbox://nobody@Local%20Folders/ColumnsSent
Anonymous id=mailbox://nobody@Local%20Folders/ColumnsVirtual
Anonymous id=mailbox://nobody@Local%20Folders/Inbox
Anonymous id=mailbox://nobody@Local%20Folders/MessageSizeA
Anonymous id=mailbox://nobody@Local%20Folders/PaneFocus
Anonymous id=mailbox://nobody@Local%20Folders/RightClickMiddleClickA
Anonymous id=mailbox://nobody@Local%20Folders/RightClickMiddleClickB
Anonymous id=mailbox://nobody@Local%20Folders/SearchBoth
Anonymous id=mailbox://nobody@Local%20Folders/Search1
Anonymous id=mailbox://nobody@Local%20Folders/Search2
Anonymous id=mailbox://nobody@Local%20Folders/SummarizationA
Anonymous id=mailbox://nobody@Local%20Folders/Test
Anonymous id=mailbox://nobody@Local%20Folders/Trash
Anonymous id=mailbox://nobody@Local%20Folders/Unsent%20Messages
Anonymous id=prefBox
Anonymous id=pref-category-box
Anonymous id=preferencesContainer

Now I have to check which of them are used in document.getElementById().

Comment 3

[:aceman]

(In reply to :aceman from comment #2)
> Anonymous id=addonsButton
We do not reference it.

> Anonymous id=category-box
We do not reference it. But I rename this one to pref-category-box (in preferences) as there is already 'category-box' in addons page. This makes it clear we do not call getElementById() on this one, but on the addons' one.

> Anonymous id=dialogOverlay-0
Needs fix.

> Anonymous id=dialogStack
Needs fix.

> Anonymous id=dialogTemplate
Needs fix.

> Anonymous id=largeTooltip
Needs fix.

> Anonymous id=mailbox://nobody@Local%20Folders
> Anonymous id=mailbox://nobody@Local%20Folders/ColumnsA
> Anonymous id=mailbox://nobody@Local%20Folders/ColumnsApplyParent
> Anonymous id=mailbox://nobody@Local%20Folders/ColumnsApplyParent/Child1
> Anonymous id=mailbox://nobody@Local%20Folders/ColumnsApplyParent/Child2
> Anonymous id=mailbox://nobody@Local%20Folders/ColumnsApplySource
> Anonymous id=mailbox://nobody@Local%20Folders/ColumnsB
> Anonymous id=mailbox://nobody@Local%20Folders/ColumnsSent
> Anonymous id=mailbox://nobody@Local%20Folders/ColumnsVirtual
> Anonymous id=mailbox://nobody@Local%20Folders/Inbox
> Anonymous id=mailbox://nobody@Local%20Folders/MessageSizeA
> Anonymous id=mailbox://nobody@Local%20Folders/PaneFocus
> Anonymous id=mailbox://nobody@Local%20Folders/RightClickMiddleClickA
> Anonymous id=mailbox://nobody@Local%20Folders/RightClickMiddleClickB
> Anonymous id=mailbox://nobody@Local%20Folders/SearchBoth
> Anonymous id=mailbox://nobody@Local%20Folders/Search1
> Anonymous id=mailbox://nobody@Local%20Folders/Search2
> Anonymous id=mailbox://nobody@Local%20Folders/SummarizationA
> Anonymous id=mailbox://nobody@Local%20Folders/Test
> Anonymous id=mailbox://nobody@Local%20Folders/Trash
> Anonymous id=mailbox://nobody@Local%20Folders/Unsent%20Messages
Automatically generated by the folderpicker (folderWidgets.xml). I don't think we ever use the ids, but we will see if tests fail.

> Anonymous id=prefBox
We do not reference it.

> Anonymous id=pref-category-box
My renamed "category-box". We do not reference it.

> Anonymous id=preferencesContainer
We do not reference it.

Patch coming.

Comment 4

[:aceman]

Attachment: 1502997.patch

This should do it. I couldn't test it locally as mozmill is very unreliable here. Needs pushing to try when building is no longer busted.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 9020980 [details] [diff] [review]
1502997.patch

Review of attachment 9020980 [details] [diff] [review]:
-----------------------------------------------------------------

Still busted but this is with a trick:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=989395f2efff0bf40f067ccc96be6c5f9c6c2c5d

::: mail/components/preferences/aboutPreferences.xml
@@ +23,5 @@
>               closebuttonaccesskey="&preferencesCloseButton.accesskey;"
>               role="dialog">
>        <xul:stack flex="1">
>          <xul:hbox id="prefBox" flex="1">
> +          <xul:vbox id="pref-category-box">

Why does the renaming make any difference here? Isn't one ID as good as the other?

Comment 6

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=989395f2efff0bf40f067ccc96be6c5f9c6c2c5d

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1502997.patch

You missed test-calendar-utils.js.

There's still a test failure in testTaskView.js, but I can't fix that now.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fd1f4b9f6638
Do not use GetElementById() to find anonymous elements after bug 1502875. r=jorgk DONTBUILD

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #5)
> > +          <xul:vbox id="pref-category-box">
> Why does the renaming make any difference here? Isn't one ID as good as the
> other?
Or maybe that's to resolve a clash with
https://searchfox.org/comm-central/rev/d5425aa4415b8204fc20c446e2b609b40d15df62/mail/themes/shared/mail/extensionsOverlay.css#6

Comment 10

[Jorg K (CEST = GMT+2)]

Actually, two failures, I didn't quite fix the first one :-(
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/testAlarmDefaultValue.js | testAlarmDefaultValue.js::testDefaultAlarms
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/views/testTaskView.js | testTaskView.js::testTaskView

Comment 11

[:aceman]

(In reply to Jorg K (GMT+2) from comment #5)
> ::: mail/components/preferences/aboutPreferences.xml
> @@ +23,5 @@
> > +          <xul:vbox id="pref-category-box">
> 
> Why does the renaming make any difference here? Isn't one ID as good as the
> other?

> Or maybe that's to resolve a clash with
> https://searchfox.org/comm-central/rev/
> d5425aa4415b8204fc20c446e2b609b40d15df62/mail/themes/shared/mail/
> extensionsOverlay.css#6

Yes I said that, it's with https://searchfox.org/comm-central/source/mozilla/toolkit/mozapps/extensions/content/extensions.xul#126
So that it is clear the getElementById() at https://searchfox.org/comm-central/source/mail/base/content/aboutAddonsExtra.js#42 finds the addon page element, not the one in prefs.
But it is just a suggestion.

Comment 12

[Richard Marti (:Paenglab)]

This shouldn't be needed to rename. The stylesheets are loaded on their pages and don't look on other pages.

Comment 13

[:aceman]

Of course:) It is just for our readability. There is also another anonid="category-box" in calendar.

Comment 14

[Jorg K (CEST = GMT+2)]

I'm in favour of renaming. That makes global changes much easier. Using the same vanilla name everywhere is very inconvenient. Best example is right here. One calendar site was overlooked since it wasn't easily distinguishable.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9021106 [details] [diff] [review]
1502997.patch

OK, pending follow-up.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1502997-follow-up-1.patch

Provided by Aceman. This fixes testAlarmDefaultValue.js.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9da5d0b8fa80
Access pref-category-box id properly for an anonymous element. rs=bustage-fix,jorgk

Comment 18

[:aceman]

Attachment: 1502997-2.patch

This should fix the remaining test. I don't know if the whole path to the element is needed, but this finally worked after a few hours of debugging :(

Comment 19

[:aceman]

I have now run calendar tests through my modified Element::AddToIdTable() and there are these anonymous elements with id (seen by tests), that are new:
Anonymous id=hbox
Anonymous id=priority-0-menuitem
Anonymous id=priority-1-menuitem
Anonymous id=priority-5-menuitem
Anonymous id=priority-9-menuitem
Anonymous id=timepicker-text

All seem OK after the fix of priority-1-menuitem in the last patch.

Comment 20

[Geoff Lankow (:darktrojan)]

We really shouldn't have anonymous content with an id attribute. The whole point of a binding is that it can be reused, and the point of an id is that there's only one of them. Still, calendar is full of bindings that shouldn't be there at all…

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/02c01fa93db9
Access "priority-1-menuitem" id properly for an anonymous element. r=darktrojan

Comment 22

[Jorg K (CEST = GMT+2)]

Changed indentation to four spaces ;-)