Emil Ghitta, QA [:emilghitta] Reporter
	Description • 6 years ago

[Affected versions]:
Firefox 62.0a1 (BuildId:20180515220059).
Firefox 61.0b5 (BuildId:20180514150347).

[Affected platforms]:
Windows 10 64bit.
macOS 10.12
Ubuntu 16.04 64bit

[Steps to reproduce]:
1. Launch Firefox.
2. Access any website.
3. Enable the Accessibility features.
4. Expand several elements from the "Properties" panel.
5. Dock the "Developer Tools" panel to side.

[Expected result]:
The panel displays the same information as before changing the position.

[Actual result]:
Some information is not displayed under the "Properties panel".

[Regression range]:
I don't think that this is a regression.

[Additional information]:
For further information regarding this issue please observe the attached screencast.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 1 • 6 years ago

This ensures that scrollTop value is in sync with scroll state when re-docking the toolbox.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 2 • 6 years ago

Second alternative but this currently fails tree test 11 were focused node should move into view when arrowing down on last visible one.

	Nicolas Chevobbe [:nchevobbe]
	Comment 3 • 6 years ago

Yura, should I review both patches ?

	Nicolas Chevobbe [:nchevobbe]
	Comment 4 • 6 years ago

Comment on attachment 8980410 [details] [diff] [review]
1461977 alternative 1

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

This change looks good to me, and I prefer it to alternative 2 because it's more explicit what it is doing.
We also need a test to make sure the bug is fixed and that we don't regress it.

::: devtools/client/shared/components/VirtualizedTree.js
@@ +345,2 @@
>     */
> +  _updateGeometry() {

nit: not sure about the name here, it isn't obvious what it does unless you look into it (maybe updateHeightAndScrollPosition ? )

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 5 • 6 years ago

This fixes the issue with scrollTop being re-set and instead syncs it up with the scroll state that we want to preserve on-resize.

	Nicolas Chevobbe [:nchevobbe]
	Comment 6 • 6 years ago

Comment on attachment 8980753 [details] [diff] [review]
1461977 v2

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

I tested the patch and this seems good to me.
Again, it would be nice if there were a test to ensure we do fix the bug and we don't regress :)

::: devtools/client/shared/components/VirtualizedTree.js
@@ +459,5 @@
> +    // When tree size changes without direct user action, scroll top cat get re-set to 0
> +    // (for example, when tree height changes via CSS rule change). We need to ensure that
> +    // the tree's scrollTop is in sync with the scroll state.
> +    if (this.state.scroll !== this.refs.tree.scrollTop) {
> +      this.refs.tree.scrollTo(0, this.state.scroll);

nit: would you mind doing it the option way so it's easier to get what's done directly: 
scrollTo({left: 0, top: this.state.scroll})

?

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 7 • 6 years ago

Carrying over r+

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 8 • 6 years ago

Hi Emil, when this lands on nightly could you verify that the bug is fixed for you along with the bug 
1443155
?

	Pulsebot
	Comment 9 • 6 years ago

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8695f808fb0
sync tree's scrollTop with scroll state when tree scrollTop value changes outside of user scroll. r=nchevobbe

	Stefan Hindli [:stefan_hindli]
	Comment 10 • 6 years ago
bugherder

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

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 11 • 6 years ago

Comment on attachment 8981218 [details] [diff] [review]
1461977 patch v3

[Feature/Bug causing the regression]: Not a regression, one of the high priority bugs for a11y inspector panel feature
[User impact if declined]: If declined, as described in the bug (+ related bug) there are times when properties sidebar gets rendered as half empty and looks broken.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Waiting for confirmation.
[Needs manual test from QE? If yes, steps to reproduce]: Same steps as in the description.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: All the change does is scrolling the properties in sidebar to an original position.
[String changes made/needed]: None

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 6 years ago

Comment on attachment 8981218 [details] [diff] [review]
1461977 patch v3

a11y explorer fix, approved for 61.0b10.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/5e583b4b75c6

	Emil Ghitta, QA [:emilghitta] Reporter
	Comment 14 • 6 years ago

This issue is verified fixed using Firefox 62.0a1 (BuildID:20180529220039) on Windows 10 64bit, macOS 10.10.5 and Ubuntu 16.04 64bit.

Leaving the ni on myself as a reminder to verify this on 61.0b10 as well.

	Emil Ghitta, QA [:emilghitta] Reporter
	Comment 15 • 6 years ago

This issue is verified fixed using Firefox 61.0b10 (BuildId:20180530184300) on Windows 10 64bit, macOS 10.12.6 and Ubuntu 16.04 64bit.