799077 - Add transitions when sliding a breakpoint to another line

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

In bug 737803 comment 8 Mihai suggested that we need changes in Orion to implement this. I'm filing this bug to make sure we do so, after bug 759351 or any other dependencies land. I'd prefer the icon sliding to the new line, but a fade out/in effect works for me, too.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Is this easier now that we use code mirror?

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Also +1 for sliding.

Comment 3

[Mihai Sucan [:msucan]]

Difficulty seems similar to the Orion days. Might actually be easy with CSS animations, it needs some time to play with CSS. This could be a mentored bug.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Yeah, hopefully CM will make this easier.

Comment 5

[James Long (:jlongster)]

Attachment: 799077-bp-transition.patch

This wasn't too hard. The only thing I don't like is that I had to embed the CSS style for transitions in JS. This is because CSS transitions suck. I change the "top" style of the element after adding a transition, which makes it slide, and in the "transitionend" event remove the old breakpoint and add the new one.

Because we keep the actual breakpoint element around (when removing, we just remove the "class" breakpoint), I need to reset the div element with a "top" of 0. However since it has a transition, it doesn't happen immediately and something in the removal is stopping it from happening. The result is that once a breakpoint shifts, it always stays shifted. I fixed it by only applying a transition property when I want it to happen.

Comment 6

[Victor Porof [:vporof][:vp]]

Drive-by, hope you don't mind :)

(In reply to James Long (:jlongster) from comment #5)
> Created attachment 8404005 [details] [diff] [review]
> 799077-bp-transition.patch

> I fixed it by only applying a transition property when I want it to happen.

Wouldn't simply using an attribute work? Like

> .breakpoint[sliding] {
>   transition: top .25s;
> }

..and add/remove the attribute when it makes sense.

Also, I cry a little whenever I see "top" used instead of "translate". If it "feels" smoother using a transformation here, I'd vote for using it and avoid needless reflows caused by "top".

Comment 7

[James Long (:jlongster)]

(In reply to Victor Porof [:vporof][:vp] from comment #6)
> Drive-by, hope you don't mind :)
> 
> (In reply to James Long (:jlongster) from comment #5)
> > Created attachment 8404005 [details] [diff] [review]
> > 799077-bp-transition.patch
> 
> > I fixed it by only applying a transition property when I want it to happen.
> 
> Wouldn't simply using an attribute work? Like
> 
> > .breakpoint[sliding] {
> >   transition: top .25s;
> > }
> 
> ..and add/remove the attribute when it makes sense.

I tried that too, and it didn't work at a quick glance. I can try it again. There's something funky with the interactions of transitions. I always have these problems with CSS transitions...

> 
> Also, I cry a little whenever I see "top" used instead of "translate". If it
> "feels" smoother using a transformation here, I'd vote for using it and
> avoid needless reflows caused by "top".

You're right, I can change it. It seems smooth enough, but we should use translate. When are we getting `will-change`?

Comment 8

[Victor Porof [:vporof][:vp]]

(In reply to James Long (:jlongster) from comment #7)
> (In reply to Victor Porof [:vporof][:vp] from comment #6)
> > Drive-by, hope you don't mind :)
> > 
> > (In reply to James Long (:jlongster) from comment #5)
> > > Created attachment 8404005 [details] [diff] [review]
> > > 799077-bp-transition.patch
> > 
> > > I fixed it by only applying a transition property when I want it to happen.
> > 
> > Wouldn't simply using an attribute work? Like
> > 
> > > .breakpoint[sliding] {
> > >   transition: top .25s;
> > > }
> > 
> > ..and add/remove the attribute when it makes sense.
> 
> I tried that too, and it didn't work at a quick glance. I can try it again.
> There's something funky with the interactions of transitions. I always have
> these problems with CSS transitions...
> 

Yeah, they can be a PITA sometimes. You could quickly take a look at how the debugger does the "bounce" animation when clicking on a match, and see if it rings any bells [0] [1]

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#2900
[1] http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/debugger.inc.css?from=debugger.inc.css#472

> > 
> > Also, I cry a little whenever I see "top" used instead of "translate". If it
> > "feels" smoother using a transformation here, I'd vote for using it and
> > avoid needless reflows caused by "top".
> 
> You're right, I can change it. It seems smooth enough, but we should use
> translate. When are we getting `will-change`?

FWIW, (sadly) even if we did get "will-change", I don't think that will actually make any difference with archaic properties like "top". Anyway, off-topic :P

Comment 9

[James Long (:jlongster)]

Attachment: 799077-bp-transition.patch

This uses transforms instead. I have to leave now and will be on PTO for a week, I'll figure out how to get the transition into the CSS when I get back.

Comment 10

[Mihai Sucan [:msucan]]

James, patch looks good to me. Feel free to ask again for review when it's ready. Thanks!

Comment 11

[James Long (:jlongster)]

Attachment: 799077-bp-transition.patch

Final patch, I moved the transition into CSS. All tests pass.

Comment 12

[James Long (:jlongster)]

Attachment: 799077-bp-transition.patch

Haven't seen msucan for a few days; panos mind reviewing this? it's a really small patch. Uploaded a new one to fix a minor coding style.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8418851 [details] [diff] [review]
799077-bp-transition.patch

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

Love it!

::: browser/devtools/debugger/debugger-controller.js
@@ +1745,5 @@
>  
>      // Initialize the breakpoint, but don't update the editor, since this
>      // callback is invoked because a breakpoint was added in the editor itself.
>      this.addBreakpoint(location, { noEditorUpdate: true }).then(aBreakpointClient => {
>        // If the breakpoint client has an "requestedLocation" attached, then

Can you also fix the "an" typo above, please?

::: browser/devtools/sourceeditor/debugger.js
@@ +167,5 @@
> +    marker.addEventListener('transitionend', function(e) {
> +      ed.removeBreakpoint(info.line);
> +      ed.addBreakpoint(toLine);
> +
> +      // For some reason, we have to do reset the styles after the

Typo: "do" is redundant here.

::: browser/devtools/sourceeditor/editor.js
@@ +508,5 @@
>    /**
>     * Returns whether a marker of a specified class exists in a line's gutter.
>     */
>    hasMarker: function (line, gutterName, markerClass) {
> +    let marker = this.getMarker(line, gutterName, markerClass);

getMarker() only takes 2 parameters.

Comment 14

[James Long (:jlongster)]

Attachment: 799077-bp-transition.patch

Thanks, fixed! Waiting on try now.

Comment 15

[James Long (:jlongster)]

Tests are passing on try: https://tbpl.mozilla.org/?tree=Try&rev=6f6d4a170d2b

Comment 16

[James Long (:jlongster)]

Attachment: 799077-bp-transition.patch

Add bug/message to patch

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/68b54292da41

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68b54292da41