1236991 - Update or remove tests that use fillInPageTooltip

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

browser_bug329212.js seems to be concerned with SVG element tooltips, and calls popup.xml#tooltip's "fillInPageTooltip" method to populate the tooltip label given the SVG node.

With e10s enabled, this forces us to use CPOWs to send the node to fillInPageTooltip. This test breaks when we outlaw CPOWs (bug 1233497).

I'm not sure this test adds much value. As far as I can tell, fillInPageTooltip isn't used by anything but tests anyways (except for browser.js's "FillInHTMLTooltip", which isn't ever called by anyone).

I've disabled the test for e10s for now, but we might just want to get rid of it entirely (as well as fillInPageTooltip).

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Actually, it seems there are a number of tests that use fillInPageTooltip. They are:

browser_bug329212.js
browser_bug331772_xul_tooltiptext_in_html.js
browser_bug561623.js
browser_bug581947.js
browser_input_file_tooltips.js

These tests will just break when CPOWs go away and should be re-written to use a different mechanism to exercise tooltips.

Comment 2

[Neil Deakin]

While the global fillInPageTooltip function in browser.js is deprecated, the code for it was moved into popup.xml, and the functionality still needs to be tested.

In e10s though, a different codepath is used that does more or less the same thing is used instead (in nsDocShellTreeOwner.cpp).

These tests should be rewitten to open up real tooltips and then verify their contents which would handle both cases.

Ideally, of course, both codepaths would also be merged but that is a separate issue.

Comment 3

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #2)
> While the global fillInPageTooltip function in browser.js is deprecated, the
> code for it was moved into popup.xml, and the functionality still needs to
> be tested.
> 
> In e10s though, a different codepath is used that does more or less the same
> thing is used instead (in nsDocShellTreeOwner.cpp).
> 
> These tests should be rewitten to open up real tooltips and then verify
> their contents which would handle both cases.
> 
> Ideally, of course, both codepaths would also be merged but that is a
> separate issue.

The tests actually in several cases test that the tooltip is empty and not shown for particular elements. That's not really testable, because tooltips are not shown synchronously, and while you can wait for a popupshown event for the tooltip to be shown, you can't really determine when a tooltip would not be shown.

Is there a better way to do this?

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Neil Deakin from comment #2)
> > While the global fillInPageTooltip function in browser.js is deprecated, the
> > code for it was moved into popup.xml, and the functionality still needs to
> > be tested.
> > 
> > In e10s though, a different codepath is used that does more or less the same
> > thing is used instead (in nsDocShellTreeOwner.cpp).
> > 
> > These tests should be rewitten to open up real tooltips and then verify
> > their contents which would handle both cases.
> > 
> > Ideally, of course, both codepaths would also be merged but that is a
> > separate issue.
> 
> The tests actually in several cases test that the tooltip is empty and not
> shown for particular elements. That's not really testable, because tooltips
> are not shown synchronously, and while you can wait for a popupshown event
> for the tooltip to be shown, you can't really determine when a tooltip would
> not be shown.
> 
> Is there a better way to do this?

Per discussion with Neil on IRC, it's unclear that there's a way to wait for a tooltip to not be shown, so instead I'll look at unifying the code here and then fixing the tests to test the same codepath in e10s and non-e10s.

Comment 5

[:Gijs (he/him)]

nsDocShellTreeOwner's code actually behaves subtly different from the popup.xml code (in particular, it doesn't currently deal with SVG <title> elements inside SVG <a> elements -- there might be other bugs I'll uncover when adapting these tests), so we should probably fix this sooner rather than later. Requesting tracking to keep this on the radar.

Comment 6

[Neil Deakin]

You should assume that the popup.xml version is correct.

Comment 7

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #6)
> You should assume that the popup.xml version is correct.

Right, and so I did - hence requesting tracking-e10s, because that means a number of edgecases are due to regress with e10s unless we fix them. :-)

Comment 8

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1236991 - part 1: make default tooltiptextprovider xpcom-accessible, r?enndeakin

Review commit: https://reviewboard.mozilla.org/r/45493/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45493/

Comment 9

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1236991 - part 2: use the xpcom tooltiptextprovider from popup.xml, r?enndeakin

Review commit: https://reviewboard.mozilla.org/r/45495/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45495/

Comment 10

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1236991 - part 3: fix tooltips picking up SVG <title> elements inside SVG link tags, r?enndeakin

Review commit: https://reviewboard.mozilla.org/r/45497/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45497/

Comment 11

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1236991 - part 4: add XUL tooltiptext code to tooltiptextprovider code, r?enndeakin

Review commit: https://reviewboard.mozilla.org/r/45499/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45499/

Comment 12

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1236991 - part 5: <output> elements don't submit, and so shouldn't get form validation errors in tooltips, r?enndeakin

Review commit: https://reviewboard.mozilla.org/r/45501/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45501/

Comment 13

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1236991 - part 3: allow use of todo() from ContentTask, r=mconley

Review commit: https://reviewboard.mozilla.org/r/45503/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45503/

Comment 14

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1236991 - part 4: fix the tests to run in e10s mode, r?enndeakin

Review commit: https://reviewboard.mozilla.org/r/45505/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45505/

Comment 15

[Neil Deakin]

I'd actually expected you to make a JS component out of the popup.xml code. It doesn't matter I guess, but was there a reason not to?

Comment 16

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #15)
> I'd actually expected you to make a JS component out of the popup.xml code.
> It doesn't matter I guess, but was there a reason not to?

I did consider that option. The reason I didn't was the comment in popup.xml:

https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/popup.xml#533-535

           Note that DefaultTooltipTextProvider::GetNodeText() from nsDocShellTreeOwner.cpp
           also performs the same function, but for embedded clients that don't use a XUL/JS
           layer. These two should be kept synchronized.

I assumed that using a JS component when there wasn't a JS layer wasn't the right thing to do.

It also just seemed kind of wrong to make embedding/ depend on the toolkit-provided JS component, and embedding itself seemed to have no existing JS components - I wouldn't know where/how to get them packaged from there.

Comment 17

[Neil Deakin]

> I assumed that using a JS component when there wasn't a JS layer wasn't the
> right thing to do.

I think that comment refers to a xul browser ui. There are many JS components that would be available in an embedded environment. Look for .manifest files along with their corresponding .js file.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8740023 [details]
MozReview Request: Bug 1236991 - part 3: allow use of todo() from ContentTask, r=mconley

https://reviewboard.mozilla.org/r/45503/#review42405

Comment 19

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1236991 - part 1: allow forwarding label direction through nsITooltipTextProvider, r?enndeakin

Review commit: https://reviewboard.mozilla.org/r/45847/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45847/

Comment 20

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1236991 - part 2: implement a default tooltiptextprovider in toolkit, r?enndeakin

Review commit: https://reviewboard.mozilla.org/r/45849/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45849/

Comment 21

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1236991 - bonus: move all the tests into the component dir instead of cluttering up browser/.../tests/general, r?enndeakin

Review commit: https://reviewboard.mozilla.org/r/45851/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45851/

Comment 22

[:Gijs (he/him)]

Comment on attachment 8740023 [details]
MozReview Request: Bug 1236991 - part 3: allow use of todo() from ContentTask, r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45503/diff/1-2/

Comment 23

[:Gijs (he/him)]

Comment on attachment 8740024 [details]
MozReview Request: Bug 1236991 - part 4: fix the tests to run in e10s mode, r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45505/diff/1-2/

Comment 24

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #17)
> > I assumed that using a JS component when there wasn't a JS layer wasn't the
> > right thing to do.
> 
> I think that comment refers to a xul browser ui. There are many JS
> components that would be available in an embedded environment. Look for
> .manifest files along with their corresponding .js file.

Done. I stuck it in toolkit/, but I guess we could put it somewhere else if you think that makes more sense. I use hg cp on popup.xml so it'd be easier to see what I changed, and it kind of worked... I had to change a number of the references to DOM globals (Node, SVGElement, etc.) to rely on the tipElement's defaultView, and I refactored the final lines to not set this.label / this.style.direction but to return the two (see also the patch labeled "part 1"), but it's otherwise unchanged.

Comment 25

[Neil Deakin]

Comment on attachment 8740649 [details]
MozReview Request: Bug 1236991 - bonus: move all the tests into the component dir instead of cluttering up browser/.../tests/general, r?enndeakin

https://reviewboard.mozilla.org/r/45851/#review42961

You want to remove title_test.svg from browser.ini as well.

Comment 26

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #25)
> Comment on attachment 8740649 [details]
> MozReview Request: Bug 1236991 - bonus: move all the tests into the
> component dir instead of cluttering up browser/.../tests/general, r?enndeakin
> 
> https://reviewboard.mozilla.org/r/45851/#review42961
> 
> You want to remove title_test.svg from browser.ini as well.

It is also used from https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/offlineQuotaNotification.cacheManifest which is in turn used in some offline quota tests. :-(

Comment 27

[Neil Deakin]

Comment on attachment 8740647 [details]
MozReview Request: Bug 1236991 - part 1: allow forwarding label direction through nsITooltipTextProvider, r?enndeakin

https://reviewboard.mozilla.org/r/45847/#review42963

The direction handling is actually bug 1169648 that was never finished. Perhaps coordinate there to make sure there isn't anything missing here.

Comment 28

[Neil Deakin]

Comment on attachment 8740024 [details]
MozReview Request: Bug 1236991 - part 4: fix the tests to run in e10s mode, r?enndeakin

https://reviewboard.mozilla.org/r/45505/#review42965

Comment 29

[Neil Deakin]

The try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f6e3151904f&selectedJob=19378728 has some failures. Is that reflective of the latest patches and should I wait to finishing reviewing here?

Comment 30

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #29)
> The try run at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3f6e3151904f&selectedJob=19378728 has some failures.
> Is that reflective of the latest patches and should I wait to finishing
> reviewing here?

That's surprising. I guess you can wait while I update for some of these review comments and try to work out why the test isn't working - I'm fairly sure I ran it locally and it worked, but clearly it's not working on infra... :-\

Comment 31

[:Gijs (he/him)]

Comment on attachment 8740647 [details]
MozReview Request: Bug 1236991 - part 1: allow forwarding label direction through nsITooltipTextProvider, r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45847/diff/1-2/

Comment 32

[:Gijs (he/him)]

Comment on attachment 8740648 [details]
MozReview Request: Bug 1236991 - part 2: implement a default tooltiptextprovider in toolkit, r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45849/diff/1-2/

Comment 33

[:Gijs (he/him)]

Comment on attachment 8740023 [details]
MozReview Request: Bug 1236991 - part 3: allow use of todo() from ContentTask, r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45503/diff/2-3/

Comment 34

[:Gijs (he/him)]

Comment on attachment 8740024 [details]
MozReview Request: Bug 1236991 - part 4: fix the tests to run in e10s mode, r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45505/diff/2-3/

Comment 35

[:Gijs (he/him)]

Comment on attachment 8740649 [details]
MozReview Request: Bug 1236991 - bonus: move all the tests into the component dir instead of cluttering up browser/.../tests/general, r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45851/diff/1-2/

Comment 36

[:Gijs (he/him)]

Comment on attachment 8740648 [details]
MozReview Request: Bug 1236991 - part 2: implement a default tooltiptextprovider in toolkit, r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45849/diff/2-3/

Comment 37

[:Gijs (he/him)]

Comment on attachment 8740023 [details]
MozReview Request: Bug 1236991 - part 3: allow use of todo() from ContentTask, r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45503/diff/3-4/

Comment 38

[:Gijs (he/him)]

Comment on attachment 8740024 [details]
MozReview Request: Bug 1236991 - part 4: fix the tests to run in e10s mode, r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45505/diff/3-4/

Comment 39

[:Gijs (he/him)]

Comment on attachment 8740649 [details]
MozReview Request: Bug 1236991 - bonus: move all the tests into the component dir instead of cluttering up browser/.../tests/general, r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45851/diff/2-3/

Comment 40

[Neil Deakin]

Comment on attachment 8740647 [details]
MozReview Request: Bug 1236991 - part 1: allow forwarding label direction through nsITooltipTextProvider, r?enndeakin

https://reviewboard.mozilla.org/r/45847/#review43071

Comment 41

[Neil Deakin]

Comment on attachment 8740648 [details]
MozReview Request: Bug 1236991 - part 2: implement a default tooltiptextprovider in toolkit, r?enndeakin

https://reviewboard.mozilla.org/r/45849/#review43073

Comment 42

[Neil Deakin]

Comment on attachment 8740649 [details]
MozReview Request: Bug 1236991 - bonus: move all the tests into the component dir instead of cluttering up browser/.../tests/general, r?enndeakin

https://reviewboard.mozilla.org/r/45851/#review43075

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86c2f4fb77a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/d42f9b1e05a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e38e7b5ae7
https://hg.mozilla.org/integration/mozilla-inbound/rev/a91d2ac5476f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1418ce4fcd0b

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f90060e64a0

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/86c2f4fb77a5
https://hg.mozilla.org/mozilla-central/rev/d42f9b1e05a3
https://hg.mozilla.org/mozilla-central/rev/b6e38e7b5ae7
https://hg.mozilla.org/mozilla-central/rev/a91d2ac5476f
https://hg.mozilla.org/mozilla-central/rev/1418ce4fcd0b
https://hg.mozilla.org/mozilla-central/rev/9f90060e64a0