Skip to content

make sure popup container is an even number of pixels wide#3258

Closed
mollymerp wants to merge 1 commit into
masterfrom
popup-blurriness
Closed

make sure popup container is an even number of pixels wide#3258
mollymerp wants to merge 1 commit into
masterfrom
popup-blurriness

Conversation

@mollymerp

Copy link
Copy Markdown
Contributor

Popups with a width of an odd number of pixels appear blurry on non-retina screens. This PR ensures that the containers are an even number of pixels. At first I thought this might be related to odd-pixel transforms in placing the popup element, but ensuring even dimensions did not fix the blurriness problem.

Right now I have this code in popup.js, but I can definitely move to a separate util/dom.js function.

Here's a gif that demonstrates the behavior

blurry-popup

ref #3249

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@mollymerp

Copy link
Copy Markdown
Contributor Author

cc @noemiwalzebuck @lucaswoj @jfirebaugh

@lucaswoj

lucaswoj commented Sep 26, 2016

Copy link
Copy Markdown
Contributor

Did you ever come to an understanding of the root cause of this bug? An adjustment to the translation is preferable to an adjustment to the width.

There are legitimate use cases for odd # widths.

I could see a developer being confused that the width they set on their popup was overwritten by GL JS whenever the map moves.

@mollymerp

mollymerp commented Sep 27, 2016

Copy link
Copy Markdown
Contributor Author

Changing the translation unfortunately doesn't make the blurriness go away. I think the bug has something to do with this.

@jfirebaugh

Copy link
Copy Markdown
Contributor

The reason to use translate(50%) is to avoid a fixed value calculated from the width of the content at a particular point in time, since the width might change if the DOM is mutated or effects such as CSS :hover are applied.

The effect of this change is to fix the width at a particular point in time. It's probably slightly better than using translate with absolute pixel coordinates instead of a percentage, because the popup cannot become off-centered, but it's less than ideal.

Looking around the web for other solutions to the blurriness issue, it seems that there's a vague consensus that a) this is browser-specific and a browser bug when it occurs and b) there's not really any good workaround.

@mollymerp

Copy link
Copy Markdown
Contributor Author

Based on discussion with the rest of the maintainers, we've determined this is a bug in certain browsers and we should not fix the width of the popups in the gl-js codebase, so I'll close this PR.

If this issue is affecting you, you can fix the blurriness by setting your popup's width to a fixed, even number of pixels.

@mollymerp mollymerp closed this Sep 27, 2016
@mollymerp mollymerp deleted the popup-blurriness branch October 7, 2016 22:03
@andrewharvey

Copy link
Copy Markdown
Collaborator

Just for reference I think this is the Chrome bug https://bugs.chromium.org/p/chromium/issues/detail?id=521364

@mikeomeara1

Copy link
Copy Markdown

Sorry for a super late comment on this one but it's the best place I could find.

I understand why this was closed and why you've chosen to not implement a fix...however, for those us us with many, dynamic sized (or content variant) popups...this issue is a pretty tough one to work around.

Setting a fixed, even pixel width means the popups can't size themselves to fit their content. We've found by setting inline-table; on .mapboxgl-popup-content this keeps the popups from using fractional width, but to this point have been unable to find a reliable way to enforce "only even numbered pixel widths"...leaving us with intermittent blurry popups.

And seeing as the practical upshot of this is a +/-1px variance on any width you set on it...I for one would really like to see this fix implemented.

@OLTC-fperrin

Copy link
Copy Markdown

@mikeomeara1 I can't believe we had to do this but here is how we worked around it:

new mapboxgl.Popup({offset: 15})
    .on('open', function (pop) {
        var _container = pop.target._container;
        if(_container.offsetWidth % 2 !== 0){
            _container.style.width = (_container.offsetWidth + 1) + "px";
        }
    });

Hope it helps

@mikeomeara1

Copy link
Copy Markdown

@LTC-fperrin That's fantastic. Agreed though, it's kind of crazy that this is necessary...but in any case, that puts us into a much better spot than we were. Thanks so much for sharing.

@OLTC-fperrin

Copy link
Copy Markdown

@mollymerp @mikeomeara1 I just realised that's exactly what this PR is about. It should be /reopened /merged !

@andrewharvey

andrewharvey commented Mar 5, 2019

Copy link
Copy Markdown
Collaborator

This is the workaround I'm using with Vue (the popup is a Vue component)

 mounted () {
   // odd width popups are blurry on Chrome, this enforces even widths
   if (Math.ceil(this.$el.clientWidth) % 2) {
     this.$el.style.width = (Math.ceil(this.$el.clientWidth) + 1) + 'px'
   }
 }

@Dakad

Dakad commented Apr 2, 2019

Copy link
Copy Markdown

I have the same problem but the popup is still blurred even when width is a even number.
I have two popups with a width of 636px and one of them is blurred (Test1).

Screenshoots

Capture d'écran 2019-04-02 10 19 03
Capture d'écran 2019-04-02 10 19 13

@antonzubar

antonzubar commented Apr 17, 2019

Copy link
Copy Markdown
.mapboxgl-popup {
    position: absolute;
    top: 0;
    left: 0;
    display: -webkit-flex;
    display: flex;
    will-change: transform;
    pointer-events: none;
}

Removing will-change: transform helped in my case. Also from documentation:

Don't apply will-change to too many elements. The browser already tries as hard as it can to optimize everything... Looks like adding this property to pop-up (could be a lot of them) not a good idea.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants