Closed
Bug 963773
Opened 7 years ago
Closed 7 years ago
The Toolbutton insertion position should consider amount of scroll of PanelUI
Categories
(Firefox :: Toolbars and Customization, defect)
Tracking
()
VERIFIED
FIXED
Firefox 30
People
(Reporter: alice0775, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Whiteboard: [Australis:P3])
Attachments
(1 file)
Build Identifier: http://hg.mozilla.org/mozilla-central/rev/9d650c07b547 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140124030216 See screen capture: http://www.youtube.com/watch?v=Nwtn5BNDZ1A Steps To Reproduce: 1. Make sure that PanelUI has scrollbar when customize mode 2. Enter Customize mode 3. Scroll down panel so that empty placeholder appears 4. Attempt to place a button on the empty placeholder Actual Results: The button is placed at wrong position Expected Results: The button should be placed at correct position
Assignee | ||
Comment 1•7 years ago
|
||
Well, that took a while. But this seems to work well. There's an issue where if there's only 1 placeholder or 1 element (I forget) at the end of the panel, and you drag between there and higher up (ie causing the number of rows to change while scrolling/dragging), the scrolling/dragging at the same time causes some issues, but that can be followup fodder if we think it's worth looking into.
Attachment #8381748 -
Flags: review?(jaws)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment 2•7 years ago
|
||
Comment on attachment 8381748 [details] [diff] [review] determine offset inside scrolling container for Australis customize mode drag placeholder UI to make things work when scrolling, Review of attachment 8381748 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/src/DragPositionManager.jsm @@ +76,5 @@ > */ > find: function(aContainer, aX, aY, aDraggedItemId) { > let closest = null; > let minCartesian = Number.MAX_VALUE; > + let containerX = this._containerInfo.left; Whey do we need to change how we handle horizontal positioning? From my understanding of this bug, we should only need to change how we account for vertical offsets.
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2) > Comment on attachment 8381748 [details] [diff] [review] > determine offset inside scrolling container for Australis customize mode > drag placeholder UI to make things work when scrolling, > > Review of attachment 8381748 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/customizableui/src/DragPositionManager.jsm > @@ +76,5 @@ > > */ > > find: function(aContainer, aX, aY, aDraggedItemId) { > > let closest = null; > > let minCartesian = Number.MAX_VALUE; > > + let containerX = this._containerInfo.left; > > Whey do we need to change how we handle horizontal positioning? From my > understanding of this bug, we should only need to change how we account for > vertical offsets. The only real reason was making it symmetrical, ie computing positioning in the same way for x and y. If you prefer me to only deal with the y bits, I could do that...
Comment 4•7 years ago
|
||
Comment on attachment 8381748 [details] [diff] [review] determine offset inside scrolling container for Australis customize mode drag placeholder UI to make things work when scrolling, Ok, you can leave it as-is as long as you are comfortable with our test coverage of this.
Attachment #8381748 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4) > Comment on attachment 8381748 [details] [diff] [review] > determine offset inside scrolling container for Australis customize mode > drag placeholder UI to make things work when scrolling, > > Ok, you can leave it as-is as long as you are comfortable with our test > coverage of this. Well, comfortable... no, but not any less than I was already, and I don't see how to automatically test this properly considering the state of our drag simulation testing code. remote: https://hg.mozilla.org/integration/fx-team/rev/2e3cdc99a6ff
Flags: in-testsuite-
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
Comment 6•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2e3cdc99a6ff
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
Assignee | ||
Comment 7•7 years ago
|
||
Comment on attachment 8381748 [details] [diff] [review] determine offset inside scrolling container for Australis customize mode drag placeholder UI to make things work when scrolling, [Approval Request Comment] Bug caused by (feature/regressing bug #): Australis' customize mode dragging User impact if declined: dragging widgets when the window is too small to contain the full panel (a real possibility on small-screen devices) doesn't work well Testing completed (on m-c, etc.): local, on m-c Risk to taking this patch (and alternatives if risky): reasonably low. The fix is quite contained, but unfortunately there are no automated tests for this feature because our drag simulation test framework code isn't very good. Only way to know is to try it and see if stuff still works correctly. I did my best on that front, but it's not as much of a guarantee as a proper automated test. String or IDL/UUID changes made by this patch: none
Attachment #8381748 -
Flags: approval-mozilla-aurora?
Updated•7 years ago
|
Attachment #8381748 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 8•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/475bc4af0a4b
status-firefox29:
--- → fixed
status-firefox30:
--- → fixed
Updated•7 years ago
|
QA Contact: cornel.ionce
Comment 9•7 years ago
|
||
I reproduced this issue on a Firefox 30.0 build from February 27th. When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found some remaining issues: 1. When the last row of the panel contains only empty placeholders and you try to place a button on row X, row X+1 shift to make space for it. It's also placed on row X+1 if you actually drop the button on row X. 2. When the last row of the panel contains both empty placeholders and buttons, you can't place a new button on that last row anywhere but on the placeholder. When trying to place it before the first button on the row or between two existing buttons, the buttons move to make space for it, but when you drop it, it moves back to the Additional Tools and Features page.
QA Contact: cornel.ionce → ioana.budnar
Comment 10•7 years ago
|
||
(In reply to Ioana Budnar, QA [:ioana] from comment #9) > I reproduced this issue on a Firefox 30.0 build from February 27th. > > When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found > some remaining issues: These issues only reproduce when the window is slightly minimized, so the PanelUI has a scrollbar when in customize mode.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Ioana Budnar, QA [:ioana] from comment #9) > I reproduced this issue on a Firefox 30.0 build from February 27th. > > When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found > some remaining issues: > 1. When the last row of the panel contains only empty placeholders and you > try to place a button on row X, row X+1 shift to make space for it. It's > also placed on row X+1 if you actually drop the button on row X. I'm not sure what you mean here. What is row "X"? Can you perhaps create a screencast of what you're seeing? When having only empty placeholders in the panel and making the panel scrollable by resizing the window, when I drag and drop buttons from the toolbox to either the row of placeholders, or the row above it, or the row above that, the added buttons land where the space is created for them to land. > 2. When the last row of the panel contains both empty placeholders and > buttons, you can't place a new button on that last row anywhere but on the > placeholder. When trying to place it before the first button on the row or > between two existing buttons, the buttons move to make space for it, but > when you drop it, it moves back to the Additional Tools and Features page. I can reproduce this issue, and I'll file another bug for it (and possibly issue 1) when issue 1 is clear to me. I'm not really sure what causes this issue - if I try on Windows, I'm actually seeing the cursor change when dragging over the created empty space, indicating I can't drop there, and the "drop" handler for the drag drop code doesn't get called at all (instead, the drag gets 'cancelled' which is why the button returns to the toolbox). I don't yet understand why that is.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ioana.budnar)
Comment 12•7 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #11) > (In reply to Ioana Budnar, QA [:ioana] from comment #9) > > I reproduced this issue on a Firefox 30.0 build from February 27th. > > > > When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found > > some remaining issues: > > 1. When the last row of the panel contains only empty placeholders and you > > try to place a button on row X, row X+1 shift to make space for it. It's > > also placed on row X+1 if you actually drop the button on row X. > > I'm not sure what you mean here. What is row "X"? Can you perhaps create a > screencast of what you're seeing? http://screencast.com/t/vcf6s5cf Whilst yesterday I could reproduce this issue every time I tried, today it took many tries and a long while to get there, so I'd say this is quite highly intermittent.
Flags: needinfo?(ioana.budnar)
(In reply to Ioana Budnar, QA [:ioana] from comment #12) > (In reply to :Gijs Kruitbosch from comment #11) > > (In reply to Ioana Budnar, QA [:ioana] from comment #9) > > > I reproduced this issue on a Firefox 30.0 build from February 27th. > > > > > > When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found > > > some remaining issues: > > > 1. When the last row of the panel contains only empty placeholders and you > > > try to place a button on row X, row X+1 shift to make space for it. It's > > > also placed on row X+1 if you actually drop the button on row X. > > > > I'm not sure what you mean here. What is row "X"? Can you perhaps create a > > screencast of what you're seeing? > > http://screencast.com/t/vcf6s5cf > > Whilst yesterday I could reproduce this issue every time I tried, today it > took many tries and a long while to get there, so I'd say this is quite > highly intermittent. It's quite possible this was made better by bug 992373.
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #13) > (In reply to Ioana Budnar, QA [:ioana] from comment #12) > > (In reply to :Gijs Kruitbosch from comment #11) > > > (In reply to Ioana Budnar, QA [:ioana] from comment #9) > > > > I reproduced this issue on a Firefox 30.0 build from February 27th. > > > > > > > > When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found > > > > some remaining issues: > > > > 1. When the last row of the panel contains only empty placeholders and you > > > > try to place a button on row X, row X+1 shift to make space for it. It's > > > > also placed on row X+1 if you actually drop the button on row X. > > > > > > I'm not sure what you mean here. What is row "X"? Can you perhaps create a > > > screencast of what you're seeing? > > > > http://screencast.com/t/vcf6s5cf > > > > Whilst yesterday I could reproduce this issue every time I tried, today it > > took many tries and a long while to get there, so I'd say this is quite > > highly intermittent. > > It's quite possible this was made better by bug 992373. Yes, I hope so! However, it was intermittent before then... not sure what's going on there. I hope to find time today or otherwise tomorrow to investigate at least issue 2, and to see if I can still reproduce some form of issue 1. Setting needinfo so I don't forget...
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #14) > (In reply to Mike Conley (:mconley) from comment #13) > > (In reply to Ioana Budnar, QA [:ioana] from comment #12) > > > (In reply to :Gijs Kruitbosch from comment #11) > > > > (In reply to Ioana Budnar, QA [:ioana] from comment #9) > > > > > I reproduced this issue on a Firefox 30.0 build from February 27th. > > > > > > > > > > When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found > > > > > some remaining issues: > > > > > 1. When the last row of the panel contains only empty placeholders and you > > > > > try to place a button on row X, row X+1 shift to make space for it. It's > > > > > also placed on row X+1 if you actually drop the button on row X. > > > > > > > > I'm not sure what you mean here. What is row "X"? Can you perhaps create a > > > > screencast of what you're seeing? > > > > > > http://screencast.com/t/vcf6s5cf > > > > > > Whilst yesterday I could reproduce this issue every time I tried, today it > > > took many tries and a long while to get there, so I'd say this is quite > > > highly intermittent. > > > > It's quite possible this was made better by bug 992373. > > Yes, I hope so! However, it was intermittent before then... not sure what's > going on there. I hope to find time today or otherwise tomorrow to > investigate at least issue 2, and to see if I can still reproduce some form > of issue 1. Setting needinfo so I don't forget... Apologies for the extreme delay here. I've had no luck reproducing issue 1, but I found the root cause of issue 2, for which I've filed bug 1004255.
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in
before you can comment on or make changes to this bug.
Description
•