Closed Bug 1502997 Opened 6 years ago Closed 6 years ago

Adapt to bug 1502875 - GetElementById no longer finds anonymous elements

Categories

(Thunderbird :: General, defect)

Product:
Thunderbird
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 65.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(3 files, 1 obsolete file)

1502997.patch
6.73 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1502997-follow-up-1.patch
1.39 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
1502997-2.patch
1.67 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review 
Bug 1502875 - Don't let GetElementById work across anonymous subtree boundaries in XUL documents. r=smaug
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).
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().
(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.
Attached patch 1502997.patch (obsolete) — — Splinter Review 
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.
Attachment #9020980 - Flags: review?(jorgk)
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?
Attached patch 1502997.patch — — Splinter Review 
You missed test-calendar-utils.js.

There's still a test failure in testTaskView.js, but I can't fix that now.
Attachment #9020980 - Attachment is obsolete: true
Attachment #9020980 - Flags: review?(jorgk)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fd1f4b9f6638
Do not use GetElementById() to find anonymous elements after bug 1502875. r=jorgk DONTBUILD
(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
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
(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.
Flags: needinfo?(richard.marti)
This shouldn't be needed to rename. The stylesheets are loaded on their pages and don't look on other pages.
Flags: needinfo?(richard.marti)
Of course:) It is just for our readability. There is also another anonid="category-box" in calendar.
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 on attachment 9021106 [details] [diff] [review]
1502997.patch

OK, pending follow-up.
Attachment #9021106 - Flags: review+
Provided by Aceman. This fixes testAlarmDefaultValue.js.
Attachment #9021358 - Flags: review?(geoff)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9da5d0b8fa80
Access pref-category-box id properly for an anonymous element. rs=bustage-fix,jorgk
Attached patch 1502997-2.patch — — Splinter Review 
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 :(
Attachment #9021360 - Flags: review?(geoff)
Status: NEW → ASSIGNED
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.
Attachment #9021358 - Flags: review?(geoff) → review+
Attachment #9021360 - Flags: review?(geoff) → review+
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…
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/02c01fa93db9
Access "priority-1-menuitem" id properly for an anonymous element. r=darktrojan
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Changed indentation to four spaces ;-)
Target Milestone: --- → Thunderbird 65.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: