Closed Bug 1222820 Opened 6 years ago Closed 6 years ago

"Image preview" tooltips still show up during drag and drop (in markup-view)

Categories

(DevTools :: Inspector, defect)

defect
Not set
minor

Tracking

(firefox45 fixed)

VERIFIED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: arni2033, Assigned: bmanojkumar24, Mentored)

Details

(Whiteboard: [good first bug][lang=js][polish-backlog])

Attachments

(2 files, 2 obsolete files)

STR:   (Win7_64, Nightly 45, 32bit, ID 20151107030439, new profile)
1. Open page "about:" in a new tab
2. Open devtools -> Inspector
3. Expand all nodes in #aboutLogoContainer element
4. Drag #aboutPageList element to "about:logo" string of <img> element in markup

Result:
 Tooltip show up. That can happen accidentally.

Expectations:
 I believe tooltips shouldn't appear during drag and drop. GoogleChrome doesn't show them
Status: NEW → UNCONFIRMED
Ever confirmed: false
I believe this could be as easy as returning false in `MarkupView.prototype._isImagePreviewTarget` if `this.isDragging` is true, and adding a test for it:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/markupview/markup-view.js
Mentor: pbrosset
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][lang=js][polish-backlog]
Attached patch 1222820.patch (obsolete) — Splinter Review
Hi, I would  like to work on the bug, can you please assign the bug to me. Also can you please review the changes i have made.Thank you :)
Assignee: nobody → bmanojkumar24
Comment on attachment 8686020 [details] [diff] [review]
1222820.patch

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

Looks good, thanks!
Have you ever written automated mochitest tests? We'll need one for this. The code changes look good, but we need to make sure this will continue to work in the future, and see we need to run everytime someone makes a change, to make sure what you added doesn't break.
Let me know if you want help with the test. I can provide a skeleton file and instructions if you want.

::: devtools/client/markupview/markup-view.js
@@ +405,5 @@
>     */
>    _isImagePreviewTarget: function(target) {
>      // From the target passed here, let's find the parent MarkupContainer
>      // and ask it if the tooltip should be shown
> +    if(this.isDragging)

nit: we have an agreed formatting style in the team that we'd like to apply consistently.
In order to make this easier, we have introduced ESLint in the code base. 
Please refer to this page to know how to use it: https://wiki.mozilla.org/DevTools/CodingStandards

if (this.isDragging) {
  return false;
}
Attachment #8686020 - Flags: feedback+
Hi, I will change the code according to the formatting style. I have never written a mochitest test. Please provide me the skeleton file and instructions. Thank You.
Adding the NI? flag to remember to look at this.
Flags: needinfo?(pbrosset)
Here's some information about how to write your new test. But before this, there's something you should change in your patch:
Instead of returning false in _isImagePreviewTarget, you should return a promise. This method is expected by the rest of the code to always return a promise. So here you could do this instead:

return promise.reject(false);

Now, for the test, I suggest that you create a new test file inside devtools/client/markupview/test/
named browser_markupview_dragdrop_tooltip.js
Don't forget to also register this file inside the browser.ini file in the same directory.

In order to run your test, you can use this command:
./mach test devtools/client/markupview/test/browser_markupview_dragdrop_tooltip.js

This will launch the test runner, and open a browser window with your new test and tell you the result of the test in the terminal window.
(more information about tests here: https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests )

In terms of the test file content, here's a skeleton with comments that should help you get it started (the test opens a new URL in a tab with an image and a div, then starts dragging the div, and checks that the image tooltip can't be displayed):

/* vim: set ts=2 et sw=2 tw=80: */
/* Any copyright is dedicated to the Public Domain.
 http://creativecommons.org/publicdomain/zero/1.0/ */

"use strict";

// Test that tooltips don't appear when dragging over tooltip targets.

const TEST_URL = "data:text/html;charset=utf8,<img src=\"about:logo\" /><div>";

add_task(function*() {
  let {inspector} = yield addTab(TEST_URL).then(openInspector);
  let {markup} = inspector;

  info("Get the tooltip target element for the image's src attribute");
  let img = yield getContainerForSelector("img", inspector);
  let target = img.editor.getAttributeElement("src").querySelector(".link");

  info("Check that the src attribute of the image is a valid tooltip target");
  // Insert code here to check that the tooltip can be shown.
  // We don't need to actually show it, search for isValidHoverTarget in other tests,
  // you'll see that we use this to find out if the tooltip can be shown.

  info("Start dragging the test div");
  yield simulateNodeDrag(inspector, "div");

  info("Now check that the src attribute of the image isn't a valid target");
  // Insert code here to check that the tooltip cannot be shown.

  info("Stop dragging the test div");
  yield simulateNodeDrop(inspector, "div");

  info("Check again the src attribute of the image");
  // Insert code here to check that the tooltip can be shown.
});
Flags: needinfo?(pbrosset)
Attached patch bug1222820.patch (obsolete) — Splinter Review
Hi, can you please review the changes, and tell me where I'm going wrong.When i run the test file, i get |TEST-UNEXPECTED-FAIL| exception.
Attachment #8688391 - Flags: feedback?(pbrosset)
Comment on attachment 8686020 [details] [diff] [review]
1222820.patch

When you upload a new version of the same patch, please mark the old one as obsolete (you can do this on the upload page, before pressing submit).
Attachment #8686020 - Attachment is obsolete: true
Comment on attachment 8688391 [details] [diff] [review]
bug1222820.patch

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

::: devtools/client/markupview/markup-view.js
@@ +405,5 @@
>     */
>    _isImagePreviewTarget: function(target) {
>      // From the target passed here, let's find the parent MarkupContainer
>      // and ask it if the tooltip should be shown
> +    if(this.isDragging)

Please adjust this statement to conform with our coding style:

if (this.isDragging) {
  return promise.reject(false);
}

::: devtools/client/markupview/test/browser_markupview_dragdrop_tooltip.js
@@ +19,5 @@
> +  info("Start dragging the test div");
> +  yield simulateNodeDrag(inspector, "div");
> +
> +  info("Now check that the src attribute of the image isn't a valid target");
> +  isValid = yield markup.tooltip.isValidHoverTarget(target);

Within a task (add_task), yielding promises that reject cause an exception to be thrown.
So you'll need to try catch this and make sure that the exception is thrown. So something like this:

// Expecting the promises returned by isValidHoverTarget to reject in case
// of an invalid target.
try {
  yield markup.tooltip.isValidHoverTarget(target);
  isValid = true;
} catch (e) {
  isValid = false;
}
Attachment #8688391 - Flags: feedback?(pbrosset)
Attached patch bug1222820.patchSplinter Review
Hi, I have made the changes, now all 3 tests pass.Please review the patch :) Thanks.
Attachment #8688391 - Attachment is obsolete: true
Attachment #8688414 - Flags: review?(pbrosset)
Comment on attachment 8688414 [details] [diff] [review]
bug1222820.patch

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

Thanks. Looks good.
Attachment #8688414 - Flags: review?(pbrosset) → review+
Here's a pending try push to make sure all tests pass and this patch doesn't cause regressions: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04d5ae0dad41
There are many failures in the the previous try build, but they aren't due to your patch. I unfortunately had pushed another change at the same time.
Here's a better try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd8b01be43bf
TRY looks green enough, landing this on fx-team: https://hg.mozilla.org/integration/fx-team/rev/9b28207cf3e3
https://hg.mozilla.org/mozilla-central/rev/9b28207cf3e3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
I have reproduced this bug with Firefox Nightly 45.0a1 (Build ID: 20151108030417) on windows 8.1, 64-bit with the instructions from comment 0 .

Verified as fixed with Firefox Aurora 45.0a2 (Build ID: 20160116004013)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
QA Whiteboard: [testday-20160115]
I have reproduced this bug on Nightly 45.0a1 (2015-11-08) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Beta 45.0b3!

Build ID: 20160204142810
User Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Firefox/45.0

As this bug is verified on Windows 8.1, 64-bit(comment 16), I am marking this as verified!
Status: RESOLVED → VERIFIED
QA Whiteboard: [testday-20160115] → [testday-20160115], [testday-20160205]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.