508819 - linkedPanel consumers rely on buggy getElementById behavior

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Boris Zbarsky [:bzbarsky]]

There are certain cases in which getElementById on the document will return anonymous nodes.  We need to fix that, but the linkedPanel setup in tabbox (also used by session store) relies on it.  Patch coming up.

The blocking noms are because this blocks bug 489925.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 2

[[e-mail]]

Might this affect accessibility's link between a tabbrowser tab and browser?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  Looks like it will (or rather, that accessibility is depending on the same bug); that's apparently not tested by our unit tests...

I have no idea how to quickly restructure that code to work correctly.  Do we need a separate bug on that?

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 392951 [details] [diff] [review]
Fix

session restore part looks ok. this code is also well covered w/ tests, so not necessary here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, tests (for the session store code, not the other!) were how I discovered this issue to start with.  ;)

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 392951 [details] [diff] [review]
Fix

>diff --git a/toolkit/content/widgets/tabbox.xml b/toolkit/content/widgets/tabbox.xml

>+                  } else {
>+                    linkedPanel = ownerDoc.getElementById(linkedPanelId);
>+                  }
>+                } else {
>+                  linkedPanel = null;
>+                }

style: no brackets around single lines, else on a new line either way (we don't use this style anywhere in this file.  Otherwise, looks great, thanks for fixing!

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 392951 [details] [diff] [review]
Fix

>diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js

>+        } else {
>+          tabpanel = ownerDoc.getElementById(panelID);
>+        }

No need for this fallback here, this code is tabbrowser specific and we'll always hit the first case.

>diff --git a/toolkit/content/widgets/tabbox.xml b/toolkit/content/widgets/tabbox.xml

>+                let linkedPanel;

>+                } else {
>+                  linkedPanel = null;
>+                }

Just initialize to null and drop this block?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Or just leave it undefined, even - don't need null specifically.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

And for accessibility, can't you just special-case nsAccessibilityAtoms::linkedPanel in nsRelUtils::AddTargetFromIDRefAttr, and add the binding parent fallback as in that patch?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: With those comments addressed, accessibility fixed

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 393238 [details] [diff] [review]
With those comments addressed, accessibility fixed

r=me. Looks good to me, and still passes our a11y mochitests locally. Thanks.

BTW, I'm curious about bug 489925 but don't have access...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/2a3fd282e472

David, I've cced you on bug 489925.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 393238 [details] [diff] [review]
With those comments addressed, accessibility fixed

Requesting branch approvals.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 393238 [details] [diff] [review]
With those comments addressed, accessibility fixed

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz for release-drivers

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch merged to 1.9.1

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch merged to 1.9.0

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checking in accessible/src/xul/nsXULTabAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULTabAccessible.cpp,v  <--  nsXULTabAccessible.cpp
new revision: 1.39; previous revision: 1.38
done
Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.109; previous revision: 1.108
done
Checking in toolkit/content/widgets/tabbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbox.xml,v  <--  tabbox.xml
new revision: 1.54; previous revision: 1.53
done

Comment 18

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/87c7809e4bec

Comment 19

[Philip Chee]

The changes to nsSessionStore.js need to be ported to SeaMonkey.

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> The changes to nsSessionStore.js need to be ported to SeaMonkey.

What you really want to do is request blocking in bug 513588 rather than removing it here.

Comment 21

[Philip Chee]

Uh, I could have sworn that I didn't touch the status fields (I know better than that).

Comment 22

[Boris Zbarsky [:bzbarsky]]

Note that comment 21 clobbered the blocking flag _again_.  It's almost like there's no collision detection on that field...

Comment 23

[Misak Khachatryan]

Philip, i did port, even port of bug  Bug 509625, which is not in 1.9.1 yet for Firefox.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We have tests which cover this specific area? If yes can we trust them or how can we manually verify the fix?

Comment 25

[Boris Zbarsky [:bzbarsky]]

We have tests for the toolkit/browser parts.  No idea about the accessibility parts (and I bet we don't).

Comment 26

[Boris Zbarsky [:bzbarsky]]

Oh, and the reason this bug existed was to fix the issues that caused test failures if the fix for bug 489925 was checked in.  Since that's checked in and the tree is green, I think we're good.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Is there a particular way to verify this fix? I noted that I can't verify the fix for bug 489925 as well (it doesn't reproduce in 1.9.0 or 1.9.1 for me).

Comment 28

[Boris Zbarsky [:bzbarsky]]

> Is there a particular way to verify this fix?

I checked in the fix for bug 489925 and the tree is still green... ;)

Comment 29

[Al Billings [:abillings - ex-MoCo]]

I love you, Boris. Have my children.