Closed Bug 1251809 Opened 8 years ago Closed 8 years ago

[e10s] Fix browser_input_file_tooltip.js to run in e10s

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Depends on 4 open bugs, Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8724362 - Flags: review?(enndeakin)
Comment on attachment 8724362 [details] [diff] [review]
Patch

r?dao, whoever can get to it sooner
Attachment #8724362 - Flags: review?(dao)
Comment on attachment 8724362 [details] [diff] [review]
Patch

I'd prefer Neil to review this
Attachment #8724362 - Flags: review?(dao)
Attachment #8724362 - Flags: review?(ehsan)
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...
Blocks: 838705
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...
Attachment #8724362 - Flags: review?(ehsan) → review-
Comment on attachment 8724362 [details] [diff] [review]
Patch

I agree.
Attachment #8724362 - Flags: review?(enndeakin) → review-
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?
Flags: needinfo?(ehsan)
(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.  :-)
Flags: needinfo?(ehsan)
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.
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/
Attachment #8730497 - Flags: review?(ehsan)
Attachment #8724362 - Attachment is obsolete: true
(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 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".
Attachment #8730497 - Flags: review?(ehsan) → review+
(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++.
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.
Filed follow-ups as requested.
Depends on: 1257368, 1257372, 1257374
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.
Flags: needinfo?(jorge)
https://hg.mozilla.org/mozilla-central/rev/d31323858727
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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
Attachment #8730497 - Flags: approval-mozilla-aurora?
It looks like it isn't used by add-ons, so no need to flag for add-on compat.
Flags: needinfo?(jorge)
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+
Attachment #8730497 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
See Also: → 1236991
Depends on: 1257750
You need to log in before you can comment on or make changes to this bug.