1251809 - [e10s] Fix browser_input_file_tooltip.js to run in e10s

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

nsDocShellTreeOwner::GetNodeText is missing the code for input[type=file] when the 'title' attribute is missing that popup.xml::fillInPageTooltip has.

I've added todo's for those, as well as a try/catch for setting input.value since it is throwing an exception of "Operation is not supported".

I can file separate bugs for both of these issues if you think the rest of the changes are fine.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8724362 [details] [diff] [review]
Patch

r?dao, whoever can get to it sooner

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8724362 [details] [diff] [review]
Patch

I'd prefer Neil to review this

Comment 4

[(no longer active)]

As far as I can tell, the issue here is that the patch for bug 838705 didn't modify DefaultTooltipTextProvider::GetNodeText() the way that it was supposed to to do the same thing there, is that right?  If that's the case, I think we should start by fixing GetNodeText() accordingly, and then see if the test passes in e10s as is, and investigate why if it doesn't...

Comment 5

[(no longer active)]

Comment on attachment 8724362 [details] [diff] [review]
Patch

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

I prefer instead we fix the underlying code, or at least start there...

Comment 6

[Neil Deakin]

Comment on attachment 8724362 [details] [diff] [review]
Patch

I agree.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment #0 was partially wrong. It does look like the implementations for these two functions are different, but this is not the cause of the bug.

popup.xml's fillInPageTooltip is called for both non-e10s and e10s browsers. In the "tooltip" binding of popup.xml there is a popupshowing event listener that calls fillInPageTooltip with the triggerNode of the tooltip. In non-e10s, the triggerNode is the element of the content document that is triggering the tooltip. In e10s, the triggerNode is the <browser> element. We can't use triggerNode like this because it represents an obvious CPOW.

This is all called from nsXULTooltipListener::ShowTooltip, which is reached through multiple steps but starts out at nsRootBoxFrame::AddTooltipSupport. Inside of nsXULTooltipListener::ShowTooltip, the triggerNode comes from mTargetNode which is set in the MouseMove event handler of nsXULTooltipListener. In the case of e10s, since the event listener is attached to the <browser>, mTargetNode is always the <browser>.

A hacky solution would be to get the event coordinates in the popupshowing event, and then use document.elementFromPoint() through a frame script to get the element that was moused over, but that seems both inefficient and potentially producing the wrong element.

Ehsan, how do you recommend I fix this?

Comment 8

[(no longer active)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Comment #0 was partially wrong. It does look like the implementations for
> these two functions are different, but this is not the cause of the bug.
> 
> popup.xml's fillInPageTooltip is called for both non-e10s and e10s browsers.
> In the "tooltip" binding of popup.xml there is a popupshowing event listener
> that calls fillInPageTooltip with the triggerNode of the tooltip. In
> non-e10s, the triggerNode is the element of the content document that is
> triggering the tooltip. In e10s, the triggerNode is the <browser> element.
> We can't use triggerNode like this because it represents an obvious CPOW.

Makes sense.

> This is all called from nsXULTooltipListener::ShowTooltip, which is reached
> through multiple steps but starts out at nsRootBoxFrame::AddTooltipSupport.
> Inside of nsXULTooltipListener::ShowTooltip, the triggerNode comes from
> mTargetNode which is set in the MouseMove event handler of
> nsXULTooltipListener. In the case of e10s, since the event listener is
> attached to the <browser>, mTargetNode is always the <browser>.
> 
> A hacky solution would be to get the event coordinates in the popupshowing
> event, and then use document.elementFromPoint() through a frame script to
> get the element that was moused over, but that seems both inefficient and
> potentially producing the wrong element.
> 
> Ehsan, how do you recommend I fix this?

It seems to me that we should run at least part of nsXULTooltipListener in the content process.  It's really meaningless to do any of this work in the parent process since as you noted, all that would give us is the <browser> element.  We normally do this based on whether a XUL root box has the tooltip attribute set, resulting in AddTooltipSupport() to be called.  It seems like that setup needs to be changed slightly.  What do we have set up in the content process?  I assume it's not a <xul:browser> element?  I think the code hosting the web content needs to setup at least the event listener bits of nsXULTooltipListener in the content process and notify the parent about the tooltip triggering node, etc.

I hope this makes enough sense, as I don't know this setup completely by heart.  :-)

Comment 9

[Neil Deakin]

That's not correct. DefaultTooltipTextProvider::GetNodeText is used to generate the tooltip text for page content in e10s and XULBrowserWindow.showTooltip from browser.js is used to show it. Neither nsXULTooltipListener nor fillInPageTooltip are relevant.

As an aside, we may want to have fillInPageTooltip return early if tipElement is a remote browser though to avoid computing a tooltip that doesn't exist.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: MozReview Request: Bug 1251809 - Add input[type=file] tooltip support for e10s. r?enn,ehsan

This patch adds support for tooltips on input[type=file] in e10s. I've updated the automated test to work in both non-e10s and e10s, but was unable to add test cases for multiple files being selected due to an issue with MockFilePicker where it only returns the first file from the returnList. Testing for multiple files will need to be done manually. Also, popopup.xml includes support for adding "And x more..." at the end of the tooltip if the selected number of files is more than our truncation amount. I was unable to add that to the remote implementation since I didn't see a way to use PluralForm-syntax strings from C++. I also didn' want to add a new string to cover this since we want to uplift these patches to 46.

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

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e764a71a9c23

Comment 12

[(no longer active)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> Created attachment 8730497 [details]
> MozReview Request: Bug 1251809 - Add input[type=file] tooltip support for
> e10s. r?enn,ehsan
> 
> This patch adds support for tooltips on input[type=file] in e10s. I've
> updated the automated test to work in both non-e10s and e10s, but was unable
> to add test cases for multiple files being selected due to an issue with
> MockFilePicker where it only returns the first file from the returnList.
> Testing for multiple files will need to be done manually.

We should be able to fix it though right?  Maybe file a follow-up?

> Also, popopup.xml
> includes support for adding "And x more..." at the end of the tooltip if the
> selected number of files is more than our truncation amount. I was unable to
> add that to the remote implementation since I didn't see a way to use
> PluralForm-syntax strings from C++. I also didn' want to add a new string to
> cover this since we want to uplift these patches to 46.

Fair!  But can you please file a follow-up for this too?  With my review comment that I will submit shortly, we should be able to unify this part of the code (but I don't think that there is a PluralForm equivalent API to use from within C++ unfortunately, which makes fixing that mildly painful.)

Comment 13

[(no longer active)]

Comment on attachment 8730497 [details]
MozReview Request: Bug 1251809 - Add input[type=file] tooltip support for e10s. r?enn,ehsan

https://reviewboard.mozilla.org/r/39951/#review36695

So I found out that HTMLInputElement::GetDisplayFileName() already does a lot of the work here.  Can you please re-use that here?  For the case where it deals with multiple files, it currently just produces "N files selected", but you can add an argument to it to customize its behavior for generating the text rendered in the control itself (its current job) and producing the tooltip.  Bonus points if you also move the popup.xml code to use that same function (but I won't hold it against you if you just want to file a follow-up for that here!)

Another question: neither of these code paths handles directory pickers.  Can you please file a follow-up for that as well?

r=me with the comments fixed.  Thanks!

::: embedding/browser/nsDocShellTreeOwner.cpp:1154
(Diff revision 1)
>          if (!content->IsAnyOfXULElements(mTag_dialog,
>                                           mTag_dialogheader,
>                                           mTag_window)) {
>            // first try the normal title attribute...
>            if (!content->IsSVGElement()) {
> -            currElement->GetAttribute(NS_LITERAL_STRING("title"), outText);
> +            nsCOMPtr<nsIAtom> inputAtom = do_GetAtom("input");

You don't need to recreate an atom here, please use nsGkAtoms::input instead.

::: embedding/browser/nsDocShellTreeOwner.cpp:1163
(Diff revision 1)
> +            // we should show the current file selection.
> +            if (content->IsHTMLElement(inputAtom) &&
> +                NS_SUCCEEDED(currElement->GetAttribute(NS_LITERAL_STRING("type"), typeText)) &&
> +                typeText.EqualsLiteral("file") &&
> +                NS_SUCCEEDED(currElement->HasAttribute(NS_LITERAL_STRING("title"), &hasAttr)) &&
> +                !hasAttr) {

This can be simplified a lot:

  if (content->IsHTMLElement(nsGkAtoms::input) &&
      content->AttrValueIs(kNameSpaceID_None,
                           nsGkAtoms::type,
                           NS_LITERAL_STRING("file"),
                           eIgnoreCase) &&
      !content->HasAttr(kNameSpaceID_None,
                        nsGkAtoms::title))

Note the case insensitive comparison for @type.

::: embedding/browser/nsDocShellTreeOwner.cpp:1167
(Diff revision 1)
> +                NS_SUCCEEDED(currElement->HasAttribute(NS_LITERAL_STRING("title"), &hasAttr)) &&
> +                !hasAttr) {
> +              found = true;
> +              nsCOMPtr<nsIDOMFileList> fileList;
> +              nsCOMPtr<nsIDOMHTMLInputElement> currInputElement(do_QueryInterface(currElement));
> +              currInputElement->GetFiles(getter_AddRefs(fileList));

GetFiles() can return null.  You'd need to handle that somehow.

::: embedding/browser/nsDocShellTreeOwner.cpp:1183
(Diff revision 1)
> +              NS_ENSURE_SUCCESS(rv, rv);
> +              uint32_t listLength = 0;
> +              rv = fileList->GetLength(&listLength);
> +              NS_ENSURE_SUCCESS(rv, rv);
> +              if (listLength == 0) {
> +                currElement->HasAttribute(NS_LITERAL_STRING("multiple"), &hasAttr);

You can use HasAttr() here too.

::: embedding/browser/nsDocShellTreeOwner.cpp:1193
(Diff revision 1)
> +                  rv = bundle->GetStringFromName(MOZ_UTF16("NoFileSelected"),
> +                                                 getter_Copies(outText));
> +                }
> +                NS_ENSURE_SUCCESS(rv, rv);
> +              } else {
> +                FileList* fl = static_cast<FileList*>(fileList.get());

nsIDOMFileList is not builtinclass, so in theory this cast is incorrect in case the interface is implemented by a JS component in an add-on!  In this case I think it would be fine to keep this and make nsIDOMFileList builtinclass.

::: toolkit/content/tests/browser/browser_input_file_tooltips.js:56
(Diff revision 1)
> +    let doc = content.document;
> +    let input = doc.createElement("input");
> +    doc.body.appendChild(input);
> +    input.id = "test_input";
> +    input.setAttribute("style", "position: absolute; top: 0; left: 0;");
> +    input.clientTop; // Flush layout

Out of curiosity, is this actully needed?

::: toolkit/content/tests/browser/browser_input_file_tooltips.js:108
(Diff revision 1)
> -  }
> +}
> +
> +function createTempFile() {
> +  let file = FileUtils.getDir("TmpD", [], false);
> +  file.append("testfile_bug1251809");
> +  file.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0o644);

Nice!  I didn't know about the 0o prefix in JS.  :-)

::: toolkit/content/widgets/popup.xml:542
(Diff revision 1)
>          <parameter name="tipElement"/>
>          <body>
>          <![CDATA[
> -          // Don't show the tooltip if the tooltip node is a document or disconnected.
> +          // Don't show the tooltip if the tooltip node is a document, browser, or disconnected.
>            if (!tipElement || !tipElement.ownerDocument ||
> +              tipElement.tagName == "xul:browser" ||

This won't do the right thing if you use XML namespaces right?  In that case the tag name could just be "browser".

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Ehsan Akhgari from comment #13)
> So I found out that HTMLInputElement::GetDisplayFileName() already does a
> lot of the work here.  Can you please re-use that here?  For the case where
> it deals with multiple files, it currently just produces "N files selected",
> but you can add an argument to it to customize its behavior for generating
> the text rendered in the control itself (its current job) and producing the
> tooltip.  Bonus points if you also move the popup.xml code to use that same
> function (but I won't hold it against you if you just want to file a
> follow-up for that here!)

I don't see how using GetDisplayFileName will help here. GetDisplayFileName will fill the out parameter with the filename if only one file is selected, otherwise it will return "X files selected." This is incorrect for other languages that may have more complicated rules around plural forms, plus it will only show a tooltip that matches already visible text, not necessarily adding any extra benefit.

I'd prefer to keep the current approach that the patch takes and file a bug to move PluralForm.jsm to a C++ implementation that can be referenced from both JS and C++.

Comment 15

[(no longer active)]

My original thinking was that we'd customize its behavior for the multi-file case in case it's being called to construct a tooltip.  However, we'd still have the issue of PluralForms lacking in C++, which I didn't pay attention to before.  So keeping the current approach sounds fine.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b031ff096388

All green with 18 triggers of M-e10s(bc2)

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/d31323858727fe06b28856e7f405485aeea3420f
Bug 1251809 - Add input[type=file] tooltip support for e10s. r=ehsan

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Filed follow-ups as requested.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Jorge, does this need the addon-compat keyword? I modified nsIDOMFileList.idl to mark it as builtinclass (per request from comment #13), which would block add-ons from creating their own implementation of this as a JS component in an add-on. I think this is highly unlikely, which is why I flagged you for needinfo instead of just setting the keyword.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d31323858727

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8730497 [details]
MozReview Request: Bug 1251809 - Add input[type=file] tooltip support for e10s. r?enn,ehsan

Approval Request Comment
[Feature/regressing bug #]: adds e10s support for tooltips on file inputs. the lack of tooltips was previously unknown due to the relative test being disabled on e10s. this patch enables the test and implements the missing parts of the feature for e10s. 
[User impact if declined]: no tooltips describing what files were picked in e10s builds. this is a regression from non-e10s builds.
[Describe test coverage new/current, TreeHerder]: this patch includes an automated test covering it. some manual testing will be needed for <input type=file multiple> when more than one file is selected.
[Risks and why]: low risk, i've retriggered try runs close to 20 times with no failures.
[String/UUID change made/needed]: none

Comment 22

[Jorge Villalobos [:jorgev] (he/him)]

It looks like it isn't used by add-ons, so no need to flag for add-on compat.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8730497 [details]
MozReview Request: Bug 1251809 - Add input[type=file] tooltip support for e10s. r?enn,ehsan

Baked on Nightly and needed for e10s, Aurora47+

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7326d5f35563

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Note that this was checked in with IGNORE IDL because the commit hook was present on aurora, though these rules have been laxed. This commit hook was just removed after landing via bug 1258577.