New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Judge resize of overlay by contentRect changing #16083

Merged
merged 1 commit into from Nov 2, 2017

Conversation

Projects
None yet
4 participants
@leroix
Contributor

leroix commented Nov 2, 2017

fix #16077

In the OverlayComponent resize observer, we're judging whether the element resized by comparing the contentRect with the result of a call to getBoundingClientRect() here. The dimensions reported by these two methods aren't always the same[1][2]. This results in a resize observer loop while the overlay decorations are present:

screen shot 2017-11-02 at 3 17 47 pm

screen shot 2017-11-02 at 3 18 18 pm

In this fix, we store the previous contentRect as reported in the ResizeObserver callback as state on the OverlayComponent. That way in subsequent calls of the ResizeObserver callback, we're comparing contentRect's to contentRect's.

@DavidLGoldberg thanks for reporting this bug and the work to help reproduce it on various builds!

/cc @nathansobo

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 2, 2017

Contributor

⚡️ Thanks @leroix @DavidLGoldberg.

Contributor

nathansobo commented Nov 2, 2017

⚡️ Thanks @leroix @DavidLGoldberg.

@nathansobo nathansobo merged commit 3a758ee into master Nov 2, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathansobo nathansobo deleted the resize-overlay-bug branch Nov 2, 2017

@DavidLGoldberg

This comment has been minimized.

Show comment
Hide comment
@DavidLGoldberg

DavidLGoldberg Nov 7, 2017

Unfortunately guys, My app is still in a state of disarray.

Oddly it still works with the baby blue dev build OR any other build with atom -d.

If it were only working with just the -d I'd say maybe I don't know how to use git or the dev mode but...I've been doing it for a while now ;)

Here's a bit more description:
DavidLGoldberg/jumpy#108

DavidLGoldberg commented Nov 7, 2017

Unfortunately guys, My app is still in a state of disarray.

Oddly it still works with the baby blue dev build OR any other build with atom -d.

If it were only working with just the -d I'd say maybe I don't know how to use git or the dev mode but...I've been doing it for a while now ;)

Here's a bit more description:
DavidLGoldberg/jumpy#108

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 7, 2017

Member

@DavidLGoldberg the commit indicates that this fix is only in Atom 1.23.0-beta0 and later...have you tried that version?

Member

50Wliu commented Nov 7, 2017

@DavidLGoldberg the commit indicates that this fix is only in Atom 1.23.0-beta0 and later...have you tried that version?

@DavidLGoldberg

This comment has been minimized.

Show comment
Hide comment
@DavidLGoldberg

DavidLGoldberg Nov 8, 2017

DavidLGoldberg commented Nov 8, 2017

@DavidLGoldberg

This comment has been minimized.

Show comment
Hide comment
@DavidLGoldberg

DavidLGoldberg Nov 8, 2017

@50Wliu @leroix @nathansobo ,

I tested on the Atom 1.23.0-beta0 and it works smoothly. Thanks for catching that @50Wliu .

I launched a 4.1.0 with a small little feature / fix and a yellow warning notification explaining the situation.

BTW, I put the notification object on the package's constructor / activation. I guess this will trigger for any new windows / reloads, but it's pretty easy to dismiss.

Is there a built in way to have this show once and only once to users? I didn't want to start fumbling with localStorage or any of that, but could it be done?

Would this be a useful feature to add into Atom's core notifications if I can get around to it?

Actually I kind of like the idea of being able to push these things to some managed bulletin but I know there are security concerns / limitations. It'd be really cool if it just read from any GH issue tagged with a certain tag :-p

DavidLGoldberg commented Nov 8, 2017

@50Wliu @leroix @nathansobo ,

I tested on the Atom 1.23.0-beta0 and it works smoothly. Thanks for catching that @50Wliu .

I launched a 4.1.0 with a small little feature / fix and a yellow warning notification explaining the situation.

BTW, I put the notification object on the package's constructor / activation. I guess this will trigger for any new windows / reloads, but it's pretty easy to dismiss.

Is there a built in way to have this show once and only once to users? I didn't want to start fumbling with localStorage or any of that, but could it be done?

Would this be a useful feature to add into Atom's core notifications if I can get around to it?

Actually I kind of like the idea of being able to push these things to some managed bulletin but I know there are security concerns / limitations. It'd be really cool if it just read from any GH issue tagged with a certain tag :-p

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 8, 2017

Contributor

Would this be a useful feature to add into Atom's core notifications if I can get around to it?

Yeah, that's a great idea. Some kind of way of identifying a notification and requesting that it only runs once would be a good feature. I'll be interested to hear your thoughts on the design.

Contributor

nathansobo commented Nov 8, 2017

Would this be a useful feature to add into Atom's core notifications if I can get around to it?

Yeah, that's a great idea. Some kind of way of identifying a notification and requesting that it only runs once would be a good feature. I'll be interested to hear your thoughts on the design.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 8, 2017

Contributor

I thought that #16086 was a fix for 1.22 but it seems like we must have missed something. I think we should follow up rather than leaving this broken for a whole release cycle. /cc @leroix

Contributor

nathansobo commented Nov 8, 2017

I thought that #16086 was a fix for 1.22 but it seems like we must have missed something. I think we should follow up rather than leaving this broken for a whole release cycle. /cc @leroix

@DavidLGoldberg

This comment has been minimized.

Show comment
Hide comment
@DavidLGoldberg

DavidLGoldberg Nov 8, 2017

Ok, so this might sound unbelievable with my limited Atom expertise (I have a hard time believing it myself) but I believe there is a much bigger issue at play.

This may have started since I did a local build of atom (with baby blue icon, I love that by the way).

But I did an install using the downloadable executable from atom.io and now have the good ole stable green icon for both

atom
and
atom -d

The weird thing is though that everytime I do atom -d Jumpy performance has no issue. My jumpy repo has a clean git status and it's equivalent to my master branch now (4.1.0).

Atom beta NON dev mode also of course works because it has the fix.

Is this all just a problem with my machine?

I only really have one dev env at the moment unfortunately.

If one of you could do
apm develop jumpy and verify that atom -d doesn't perform well I'd feel better. If it does well, then there's something scary wrong with dev mode as of late.

Right? What do I have confused?

(FYI: You have to perform a full jump now to see the 2nd iteration of the performance issue.)

-David

DavidLGoldberg commented Nov 8, 2017

Ok, so this might sound unbelievable with my limited Atom expertise (I have a hard time believing it myself) but I believe there is a much bigger issue at play.

This may have started since I did a local build of atom (with baby blue icon, I love that by the way).

But I did an install using the downloadable executable from atom.io and now have the good ole stable green icon for both

atom
and
atom -d

The weird thing is though that everytime I do atom -d Jumpy performance has no issue. My jumpy repo has a clean git status and it's equivalent to my master branch now (4.1.0).

Atom beta NON dev mode also of course works because it has the fix.

Is this all just a problem with my machine?

I only really have one dev env at the moment unfortunately.

If one of you could do
apm develop jumpy and verify that atom -d doesn't perform well I'd feel better. If it does well, then there's something scary wrong with dev mode as of late.

Right? What do I have confused?

(FYI: You have to perform a full jump now to see the 2nd iteration of the performance issue.)

-David

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment