Context menu items of Downloads Panel be invalidated after onDownloadMouseOut

VERIFIED FIXED in Firefox 52

Status

()

Firefox
Downloads Panel
VERIFIED FIXED
3 years ago
6 months ago

People

(Reporter: firefoxbugs, Assigned: George Pîrlea, Mentored)

Tracking

28 Branch
Firefox 54
Points:
2
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox52- verified, firefox53 verified, firefox54 verified)

Details

(Whiteboard: [good next bug])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8401632 [details]
screencast showing the issue

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140317233339

Steps to reproduce:

1) Download a file.
2) Click the down-pointing arrow on your toolbar. (Downloads)
3) Right-click over to the right of the item you downloaded.
4) In the menu that appears, click "Remove From History".


Actual results:

The download remains in the list - it is not removed.  Likewise, if you right-click to the left of the name (e.g. above the icon, but not on the icon) it still doesn't work.  To get it to work, I have to click near or over the name of the downloaded file or directly on the icon.


Expected results:

The "Remove From History" should not fail to work just because you didn't click in some magical place.

Firefox 28 on Kubuntu 13.10, fresh profile, all extensions & plugins disabled.

Updated

3 years ago
Status: UNCONFIRMED → NEW
Component: Untriaged → Downloads Panel
Ever confirmed: true

Updated

3 years ago
Flags: firefox-backlog+

Updated

3 years ago
Whiteboard: p=2

Updated

3 years ago
Points: --- → 2
Flags: qe-verify?
Whiteboard: p=2
As my first bug to start up with would u please assign me this bug.

Comment 2

3 years ago
You've find any clue about it? Could you share with everyone.

Or just placeholders pending? I see that you request a number of "assign me".
Flags: needinfo?(siddu244)

Comment 3

2 years ago
This is still occasionally occur with "Copy Download Link" menu on Fx42 x64 Win8.1. No error or messages in Browser Console.
Flags: needinfo?(siddu244)
OS: Linux → All
Hardware: x86 → All
Summary: Downloads: "Remove From History" doesn't work if you clicked to the right/left of the name. → Downloads Panel items menus may no effect if you clicked to the edge of menu

Comment 4

2 years ago
Although it is an [good first bug], but I did not find a way to fix it.


*this.richListBox.selectedIndex = -1;* in onDownloadMouseOut(aEvent).
*Typically, this will be the currently focused element.* on https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/command.
*clipboard.copyString(this.download.source.url);* and ohter commands call "this.download".
Summary: Downloads Panel items menus may no effect if you clicked to the edge of menu → Context menu items of Downloads Panel be invalidated after onDownloadMouseOut
Whiteboard: [good first bug]

Comment 5

2 years ago
Created attachment 8674291 [details] [diff] [review]
bug991965.diff

Tested on windows 8.1..
Could you please review this??
Attachment #8674291 - Flags: review?(mak77)

Comment 6

2 years ago
Comment on attachment 8674291 [details] [diff] [review]
bug991965.diff

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

It is always to operating the first item after onDownloadMouseOut.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/selectedIndex
Attachment #8674291 - Flags: review?(mak77) → feedback-

Comment 7

2 years ago
Created attachment 8675403 [details] [diff] [review]
Bug 991965 - Context menu items of Downloads Panel be invalidated after onDownloadMouseOut.

Please review the patch.
Attachment #8675403 - Flags: review?(paolo.mozmail)

Comment 8

2 years ago
Problem : Whenever the, we open context menu on a download item in the downloads panel, and after the context menu is opened, when we move the mouse over the context menu, a "mouseout"  event is triggered,  which sometimes set the "richList.selectedIndex"  to -1, so when we trigger an action on the context menu such as "copy download link", it doesn't work,as it doesn't have the right index.

Solution: Whenever the context menu is opened, store the selectedIndex on "contextmenuopen" event,and whenever an command on the "context menu" is triggered , restore the the saved "selectedIndex" value.

Comment 9

2 years ago
Thanks for the patch! I'll need to test in more detail before reviewing the code, and I'll be particularly busy this week, so it's possible the review would slip to next week. Alternative reviewers for this code could be Marco (:mak) or Jared (:jaws), but I don't know their availability for this week.

Comment 10

2 years ago
Can you please assign the bug to me.

Updated

2 years ago
Assignee: nobody → bmanojkumar24

Comment 11

2 years ago
Thanks for the reminder :-)

Comment 12

2 years ago
I've finally found some time to take a look at the code and test the patch locally. Thank you for your patience!

Indeed, this appears to be an issue with the logic added in bug 851519 to handle the selection on the richlistbox during keyboard navigation.

The approach followed here basically amounts to storing which item would be the target for the menu commands at the time of the right click. This mostly works, but the code in the patch would need cleanup anyways in order not to regress the case where the commands are executed not with the context menu but with another code path like a keyboard shortcut.

However, there is also a better approach, more consistent with other selection UI, for which whenever the context menu is open we inhibit the code that changes the selected item on hover. This has the advantage of keeping the "inverse" style of the originally selected item to clearly label it.

Unfortunately, independently from which approach is done, I'll not have the time required to mentor you through this bug in the following weeks. Maybe Marco, who wrote the original logic, may want to sign up for that. In case he isn't available as well, in the meantime you may consider finding another bug marked as [good first bug] with one or more developers who already signed up in the "mentors" field.
Depends on: 851519
Flags: needinfo?(mak77)

Updated

2 years ago
Attachment #8675403 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #12)
> However, there is also a better approach, more consistent with other
> selection UI, for which whenever the context menu is open we inhibit the
> code that changes the selected item on hover. This has the advantage of
> keeping the "inverse" style of the originally selected item to clearly label
> it.

I think this is a good suggestion, the current patch, as-is seems to be too much bug-prone.

I won't be fast (reviews may take a couple days) but if you wish to try implementing Paolo's suggestion I will gladly look into the patch.
Mentor: mak77@bonardo.net
Flags: needinfo?(mak77)

Updated

a year ago
Blocks: 1244854
No longer blocks: 1244854
Comment hidden (mozreview-request)
(Assignee)

Comment 15

8 months ago
I have also implemented the context menu selection mechanism that Paolo suggested in comment 12. The review request for that is in bug 1115964.

I'm a bit unsure how to proceed.
Flags: needinfo?(mak77)
Both changes should be evaluated apart imo, since they seem to make sense indipendently.
Regarding the reviewer, I'm not sure what's Paolo's availability in this period, but since he is currently already reviewing downloads panel code, he seems the best (most refreshed mind) choice at this time.

So, I'd suggest to set him as reviewer for now and I'm available as a second choice in case he's too busy to handle these in a meaningful time. He'll forward the reviews to me in case.
Flags: needinfo?(mak77)
(Assignee)

Updated

8 months ago
Attachment #8821922 - Flags: review?(paolo.mozmail)

Comment 17

8 months ago
Thanks George for the very good patches, really appreciated!

I only took a quick look for now, I'll read through them in more detail in the next few days. As Marco mentioned I'm also working on other reviews at the same time.

In the meantime, can you explain exactly which bug is fixed by each of the two patches? I'm not sure if the original description on the two bugs matches precisely, because there has been some discussion since then.
Flags: needinfo?(george)
(Assignee)

Comment 18

8 months ago
Created attachment 8824186 [details]
bad-targeting.mp4

Thanks a lot for your time, Paolo.

> In the meantime, can you explain exactly which bug is fixed by each of the two patches? I'm not sure if the original description on the two bugs matches precisely, because there has been some discussion since then.

I've roughly described which bug the patches solve in their respective commit messages.

The patch attached here solves the following problem:

In the Downloads Panel, the item you are hovering over is not always the item your actions are applied to. If you move your mouse in a particular way, you can end up in a situation where you're hovering over an item (and the hover effect is applied), but triggering an action (either by keyboard or mouse) does nothing.

I've attached a screencast (bad-targeting.mp4):

* First, I show the bug manifesting when you're using a context menu (up to second 10) 
* Then, the bug manifesting while using the keyboard – while moving my mouse over the item, I am repeatedly pressing the Delete key (from second 14)

The bad targeting is the root cause of this bug. The context menu did not cause the problem, it just surfaced it in this particular case.
Flags: needinfo?(george)

Comment 19

7 months ago
mozreview-review
Comment on attachment 8821922 [details]
Bug 991965 - Use event.target, inhibit item selection if context menu is open

https://reviewboard.mozilla.org/r/101004/#review106698

This took way too long for me to review, I'm really sorry for that. I genuinely appreciate that you took the time to contribute with this relevant fix for an outstanding user interface bug. The screencasts were really useful for me to understand the problem and the solutions. To keep a written record of that, I'll repeat what is going on exactly, even if you're already familiar with the issues.

The originalTarget -> target fix in this bug is spot on. That was clearly the main bug in the current code.

If applied without the fix for bug 1115964, however, it has the side effect of making the other bug worse. In the current version in Nightly, most of the surface of the <richlistitem> is covered with other elements, meaning that the code in onDownloadMouseOver and onDownloadMouseOut never operates unless you precisely target the one-pixel border between items. This means that the selectedItem of the richlistbox will often remain the correct one, which is the one that was automatically selected by the original right click.

With this patch, onDownloadMouseOver works "as expected", causing the commands in the menu to target the last item over which the pointer was located, instead of the one clicked when opening the menu.

This also explains why you didn't make the same originalTarget -> target fix for onDownloadMouseOut, that puzzled me initially. If you did that, the item would be deselected when hovering the menu, making the original situation described in comment 0 reappear.

I've reviewed the patch in bug 1115964 too. It completes the picture by suppressing the selection code while the context menu is open, although at that point we should definitely patch the incorrect target usage in onDownloadMouseOut too.

One thing I'm not too sure about is the new highlight behavior, in other words the CSS changes from ":hover" to [selected]. They work much better while the context menu is open, but they're probably worse if the context menu is closed and keyboard navigation is used. This is because a left click always operates on the item over which the pointer is located, regardless of which item is selected with the keyboard.

We're also changing how the hover state is presented in bug 1325574. This might require some merging depending on which bug lands first, but I can take care of that if necessary.

So, my suggestion here is to take the functional changes in the JavaScript code from bug 1115964 and merge them into a single patch in this bug. We can land these changes as soon as they're ready, and they will have no conflicts with bug 1325574. This will fix the most important functional issues, although highlight behavior will still be sub-optimal. I will ensure the fix reaches the Beta channel as soon as possible.

We can continue the discussion on the best highlight behavior in bug 1115964. Feel free to attach an updated CSS-only patch there. That bug will be less critical, although still nice to have in Firefox 52.

(In reply to George Pîrlea from comment #18)
> Thanks a lot for your time, Paolo.

Thanks to you for your time. Sorry again for not reviewing this earlier.
Attachment #8821922 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

7 months ago
Created attachment 8829107 [details]
Patch behaviour if the CSS is not applied
(Assignee)

Comment 24

7 months ago
Created attachment 8829108 [details]
Patch behaviour with CSS
(Assignee)

Comment 25

7 months ago
> This also explains why you didn't make the same originalTarget -> target fix
> for onDownloadMouseOut, that puzzled me initially. If you did that, the item
> would be deselected when hovering the menu, making the original situation
> described in comment 0 reappear.

That's not the actual reason, though. It's quite confusing, so please bear with me. Initially, I _did_ use 'target' in onDownloadMouseOut as well (it seemed like a reasonable thing to do), but I removed it because it broke the Mochitests.

The download item alerts (for potentially dangerous files) appear to depend on the 'originalTarget' behaviour. I am not entirely sure, but I think it's correct as it is – in onDownloadMouseOut, we are supposed to consider the outer, enclosing element (which is originalTarget), rather than the inner item stuff (target). In other words, 'originalTarget.parentNode == this.richListBox' makes sense and 'target.parentNode == this.richListBox' doesn't.

> One thing I'm not too sure about is the new highlight behavior, in other words
> the CSS changes from ":hover" to [selected]. They work much better while the
> context menu is open, but they're probably worse if the context menu is closed
> and keyboard navigation is used.

I've tested both keyboard and mouse navigation. See the attached 'with-css.ogv' screencast. It seems to work fine.

> This is because a left click always operates on the item over which the
> pointer is located, regardless of which item is selected with the keyboard.

I might be understanding you incorrectly.

With the current patch, if you left click an item the item you've clicked on is selected (onClick implies onMouseOver) and opened. Is this not how it's supposed to work?

Selecting items with keyboard navigation and pressing Enter works as well.


> We can land these [functional]  changes as soon as they're ready, and they
> will have no conflicts with bug 1325574. This will fix the most important
> functional issues, although highlight behavior will still be sub-optimal. I
> will ensure the fix reaches the Beta channel as soon as possible.

The functional changes are unshippable without the CSS. See the attached 'without-css.ogv' screencast to see what that would look like.

*****

To summarise what the commits do:

1) Use event.target, inhibit item selection if context menu is open 

This uses 'target' in both onDownloadMouseOver and onDownloadMouseOut, and includes the functionality for inhibiting selection when a context menu is open.

You can see that the selection UI without the CSS fixes essentially looks like before the patch (though the actual selection works correctly).

You can also see that the Mochitests don't pass/complete because of the dangerous item alerts.

2) Update CSS to show hover effect for [selected] items 

Makes the actual selection display properly. It shouldn't be too difficult to merge with bug 1325574.

3) Revert to using originalTarget in onDownloadMouseOut

Mochitests now pass.

*****

I know this was quite a confusing bug, so I really appreciate your time, Paolo. Let me know if there's something I need to clarify further or if I misunderstood something.

Comment 26

7 months ago
(In reply to George Pîrlea from comment #21)
> The alerts for potentially dangerous files (or at least the mochitests for
> that)
> seem to depend it. I can't figure out why, though.
> 
> Without this, mochitests don't complete.

Ah, good catch! It seems the current implementation of the subview relies on the selectedItem of the list view like all other commands, which doesn't seem very robust.

The simplest fix here would be to implement the inhibition of mouse events, I guess somewhere in DownloadsBlockedSubview.toggle, making sure we also restore the normal mouse behavior if the panel closes before the subview slides back in place.

You can try the subview for blocked downloads using the links at:

https://testsafebrowsing.appspot.com/

Comment 27

7 months ago
(In reply to George Pîrlea from comment #25)
> we are supposed to consider the
> outer, enclosing element (which is originalTarget), rather than the inner
> item stuff (target). In other words, 'originalTarget.parentNode ==
> this.richListBox' makes sense and 'target.parentNode == this.richListBox'
> doesn't.

It's the other way around - this depends on how XUL and XBL work. The <richlistitem> element for the download is an XBL binding implemented in "download.xml". When using the standard "target" property, an event listener located outside of the binding, in this case on the enclosing <richlistbox>, is supposed to see the binding as a black box, so the "target" is the <richlistitem>. The "originalTarget" is the element directly inside that binding, for example the <label>. This would have been the "target" if the listener was placed inside the black box.

Comment 28

7 months ago
(In reply to George Pîrlea from comment #25)
> > This is because a left click always operates on the item over which the
> > pointer is located, regardless of which item is selected with the keyboard.
> 
> I might be understanding you incorrectly.
> 
> With the current patch, if you left click an item the item you've clicked on
> is selected (onClick implies onMouseOver) and opened. Is this not how it's
> supposed to work?

The situation I'm referring to may make more sense to you now that bug 1325574 is fixed. You can rebase on the latest mozilla-central to check out the current behavior. It doesn't have the concept of the item-wide hover highlight anymore.

Ignoring the context menu case for a moment, the keyboard focus ring and the mouse highlight should be distinct and independent. On some platform configurations, the pointer itself is hidden on the screen while you're using the keyboard, but you can still use the left click. The hint to what will happen in that case is provided by the mouse hover highlight.

When the context menu is open, clicking anywhere else just closes the menu, so we might suppress the normal hover highlight and replace it with an ad-hoc item-wide highlight. But we shouldn't regress the behavior when the context menu closed.

> The functional changes are unshippable without the CSS.

Shipping the functional changes without the CSS changes does not regress the current behavior of the highlight that we already have in Nightly, but it fixes the important targeting bug. There is no reason for the latter to be dependent on the former.

While the item-wide highlight is a nice to have, it adds complexity and should be evaluated as a separate user experience change, so let's use the other bug for this. As I mentioned previously, the conclusion may still be that the current behavior is good enough, although I'm in favor of the locked item-wide highlight. The final decision also depends on how complex the CSS patch turns out to be.

Updated

7 months ago
Attachment #8821922 - Flags: review?(paolo.mozmail)

Updated

7 months ago
Attachment #8829101 - Flags: review?(paolo.mozmail)

Comment 29

7 months ago
(In reply to George Pîrlea from comment #25)
> (onClick implies onMouseOver)

This reminded me that we should check the following case:
 1. Right click on the second item
 2. Right click on the first item
 3. Check that the command in the menu is applied to the first item

Basically, I expect that the right click in step 2 should trigger an event that selects the first item, even if the original selection for onMouseOver was suppressed. The only way I could see this break is if the events happen in an unexpected order for some reason.

Thanks again for the screencasts - they made this round of review much faster!

Let me know if you have any questions or something is unclear.

Updated

7 months ago
Attachment #8829102 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8829102 - Attachment is obsolete: true
(Assignee)

Comment 32

7 months ago
Your explanation in comment 27 really clears up things. Thank you.

In reply to comment 28:

OK, I think I now understand the issue with the CSS and it's more complex than I originally thought. I agree we should treat it in bug 1115964. And yes, the functional changes are an improvement on the current situation and should be shipped even without the CSS. I apologise for my confusion.

 
> It's the other way around

Alright. Then it makes sense to use "target" in both "onDownloadMouseOver" and "onDownloadMouseOut".

> The simplest fix here would be to implement the inhibition of mouse events, I guess somewhere in DownloadsBlockedSubview.toggle, making sure we also restore the normal mouse behavior if the panel closes before the subview slides back in place.

Check the revised patch. I've implemented this and used "target" in both mouse event functions. Mouse behaviour is restored if the panel closes and all Mochitests now pass.

> Basically, I expect that the right click in step 2 should trigger an event that selects the first item, even if the original selection for onMouseOver was suppressed.

I've just tested this manually and it seems to work.
Mentor: mak77@bonardo.net → paolo.mozmail@amadzone.org

Comment 33

7 months ago
mozreview-review
Comment on attachment 8821922 [details]
Bug 991965 - Use event.target, inhibit item selection if context menu is open

https://reviewboard.mozilla.org/r/101004/#review107484

Looks good, thanks! I've edited the detailed commit message a bit (see <https://hg.mozilla.org/try/rev/c4021fc7634836a0b7739f8cf0c9524d732b18b9>), we should just describe the problems and solutions, but we don't need all the user reports and additional considerations.
Attachment #8821922 - Flags: review?(paolo.mozmail) → review+

Comment 34

7 months ago
mozreview-review
Comment on attachment 8829101 [details]
Bug 991965 - Inhibit mouse events if a DownloadsBlockedSubview is open

https://reviewboard.mozilla.org/r/106296/#review107488

While testing this, I found another existing bug due to a missing "DownloadsViewController.updateCommands();" call when opening the subview. I've rolled a fix for this into the same changeset for simplicity, updating the commit message.

I'm running a tryserver build here:

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

If this succeeds, I'll land this version of the changesets.
Attachment #8829101 - Flags: review?(paolo.mozmail) → review+

Comment 35

7 months ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aa8c3d9c71b
Use event.target and inhibit item selection if the context menu is open. r=paolo
https://hg.mozilla.org/integration/mozilla-inbound/rev/c79165ae0d02
Inhibit item selection and update commands when the DownloadsBlockedSubview is open. r=paolo

Comment 36

7 months ago
[Tracking Requested - why for this release]:
While this was an existing bug and not a regression, it's relevant because without this fix the context menu of the Downloads Panel might target the wrong item, and the "open" command in the subview for blocked downloads might not work. We should try to get this into the 52 release which is also an ESR.

Given this patch changes some event handling, I recommend some testing in Nightly before uplifting.
tracking-firefox52: --- → ?
Flags: qe-verify? → qe-verify+

Updated

7 months ago
Assignee: bmanojkumar24 → george

Comment 37

7 months ago
This turned out to be more complex than the average good first bug. Thanks George for fixing it!
Whiteboard: [good first bug] → [good next bug]

Comment 38

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5aa8c3d9c71b
https://hg.mozilla.org/mozilla-central/rev/c79165ae0d02
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
I have reproduced this bug with Nightly 31.0a1 on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Nightly 

Build   ID   20170124174107
User Agent   Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0

[bugday-20170125]

Comment 40

7 months ago
I have reproduced this bug with Nightly 31.0a1 on Windows 10,64Bit!

This bug's fix is verified with latest Nightly 

Build   ID   20170125030214
User Agent   Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0

[bugday-20170125]
(Assignee)

Comment 41

7 months ago
> This turned out to be more complex than the average good first bug. Thanks George for fixing it!

Thanks Paolo for your help and support! I really appreciate it.

Going forward, I can implement the CSS changes for bug 1115964, but someone needs to come up with an UX proposal. Do you have any recommendations for what other bugs I could at in the meantime?

Comment 42

7 months ago
(In reply to George Pîrlea from comment #41)
> Going forward, I can implement the CSS changes for bug 1115964, but someone
> needs to come up with an UX proposal. Do you have any recommendations for
> what other bugs I could at in the meantime?

Sorry for the delay. Bug 1334274 was reported recently, although I have not verified it with a public URL yet. If confirmed, this may require a bit of investigation of the code base, because we have different code paths depending on how the download was started. This can be a good one for you to work on.

I can also look up existing bugs related to downloads, but I'll need some more time. If you come across any other bug you're interested into, in any component, and you're not sure if it's a good next bug for you, feel free to ask!

Comment 43

7 months ago
Comment on attachment 8821922 [details]
Bug 991965 - Use event.target, inhibit item selection if context menu is open

Approval Request Comment
[Feature/Bug causing the regression]: Downloads Panel
[User impact if declined]: The context menu commands may fail to work or may apply to a different download than the one the user intended, and the user will often be unable to allow downloads blocked by Application Reputation checks.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes. Download multiple items from different pages, open the Downloads Panel, open the context menu of the bottom item, move the mouse slowly towards the top item, then rapidly back to the context menu. Use any context menu command like "Open In Finder" or "Remove From History" and check that it applies to the bottom download. After doing this, start a new download blocked by Application Reputation, and try to unblock it using the "Open" button from the sliding subview.
[List of other uplifts needed for the feature/fix]: Bug 1333064
[Is the change risky?]: Medium risk
[Why is the change risky/not risky?]: Risk is limited to the feature, but this should be tested thoroughly to ensure that the commands in the Downloads Panel behave are as expected. This is big improvement over the current intermittent behavior based on how the mouse pointer is moved.
[String changes made/needed]: None
Attachment #8821922 - Flags: approval-mozilla-beta?
Attachment #8821922 - Flags: approval-mozilla-aurora?

Comment 44

7 months ago
Comment on attachment 8829101 [details]
Bug 991965 - Inhibit mouse events if a DownloadsBlockedSubview is open

See comment 43.
Attachment #8829101 - Flags: approval-mozilla-beta?
Attachment #8829101 - Flags: approval-mozilla-aurora?
Comment on attachment 8821922 [details]
Bug 991965 - Use event.target, inhibit item selection if context menu is open

event handling fix for download panel, aurora53+, beta52+
Comment on attachment 8829101 [details]
Bug 991965 - Inhibit mouse events if a DownloadsBlockedSubview is open

event handling fix for download panel, aurora53+, beta52+
Attachment #8829101 - Flags: approval-mozilla-beta?
Attachment #8829101 - Flags: approval-mozilla-beta+
Attachment #8829101 - Flags: approval-mozilla-aurora?
Attachment #8829101 - Flags: approval-mozilla-aurora+

Comment 47

7 months ago
(In reply to :Paolo Amadini from comment #42)
> Sorry for the delay. Bug 1334274 was reported recently, although I have not
> verified it with a public URL yet.

This might have been a false start, unless you're interested in working in C++ on the Networking component. One JavaScript front-end alternative might be a set of bugs related to unifying the code paths we use to choose the final file name for the files we download. The logic should follow bug 753267 comment 7 and bug 753267 comment 18 regardless of how the download was started.

I also filed bug 1335403 which is a follow-up to the work and the discussions we did here and in bug 1325574, and could actually go in two different directions.

George, would you be interested in any of these?
Comment on attachment 8821922 [details]
Bug 991965 - Use event.target, inhibit item selection if context menu is open

bah.
Attachment #8821922 - Flags: approval-mozilla-beta?
Attachment #8821922 - Flags: approval-mozilla-beta+
Attachment #8821922 - Flags: approval-mozilla-aurora?
Attachment #8821922 - Flags: approval-mozilla-aurora+

Comment 49

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d83312cd0c26
https://hg.mozilla.org/releases/mozilla-aurora/rev/c10934134a85
status-firefox53: --- → fixed

Comment 50

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/7c172c2d11ba
https://hg.mozilla.org/releases/mozilla-beta/rev/6708a90583ea
status-firefox52: --- → fixed
tracking-firefox52: ? → -

Comment 51

7 months ago
Tested this issue on FF Nighlty 54.0a1(2017-02-01) with Ubuntu 16.04 and Windows 10 and I can't reproduce this issue any more. 

Note:
I couldn't reproduce this issue on Mac with any version. 
I was able to reproduce it on Windows and Ubuntu with FF Nighlty 51.0a1(2016-08-03). Please note that I couldn't reproduce this with Firefox versions from comment 39 and comment 40. Also I tested with FF Nighlty 54.0a1(2016-01-23) version before the fix to be implemented and I couldn't reproduce it.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified

Comment 52

7 months ago
(In reply to ovidiu boca[:Ovidiu] from comment #51)
> I was able to reproduce it on Windows and Ubuntu with FF Nighlty
> 51.0a1(2016-08-03). Please note that I couldn't reproduce this with Firefox
> versions from comment 39 and comment 40. Also I tested with FF Nighlty
> 54.0a1(2016-01-23) version before the fix to be implemented and I couldn't
> reproduce it.

Thanks! Yes, this is much easier to reproduce in Firefox 51 and earlier. After the redesign in Firefox 52, there is only a one pixel border between items that would trigger this bug, and you have to move the mouse very slowly over it. The issue with the blocked downloads subview should be equally easy to reproduce though.
(Assignee)

Comment 53

6 months ago
(In reply to :Paolo Amadini from comment #47)

> One JavaScript front-end alternative might
> be a set of bugs related to unifying the code paths we use to choose the
> final file name for the files we download. The logic should follow bug
> 753267 comment 7 and bug 753267 comment 18 regardless of how the download
> was started.

That sounds good. I'll take a look this weekend and see what can be done.

> I also filed bug 1335403 which is a follow-up to the work and the
> discussions we did here and in bug 1325574, and could actually go in two
> different directions.

I can work on this as well, but I'll start with the download filenames.
I reproduced the issue on old Nightly from 2016-08-03, and verified that the issue is fixed on Firefox 52 beta 4 using Windows 7 x64 and Ubuntu 16.04 x86.
status-firefox52: fixed → verified
Verified fixed Fx 53.0a2 (2017-03-06) Win 10.
status-firefox53: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.