Fix attibution controll #668

astridx · 2021-11-30T12:45:27Z

The attribute open should only added to the default view if the size is less than 640 pixel. Because only then the toggle works correct, because less than 640 the attribution control is forced to be compact (line 73).

github-actions · 2021-11-30T12:50:37Z

Bundle size report:

Size Change: +12 B
Total Size Before: 194 kB
Total Size After: 194 kB

Output file	Before	After	Change
maplibre-gl.js	185 kB	185 kB	+12 B
maplibre-gl.css	9.49 kB	9.49 kB	0 B

ℹ️ View Details

Source file	Before	After	Change
rollup/build/tsc/src/ui/control/attribution_control.js	1 kB	1.01 kB	+13 B

src/ui/control/attribution_control.ts

…o attr_fix_2

acalcutt · 2021-12-02T05:18:11Z

I tested ( bd1ee4b ) and still no luck with that version. However, comparing the html I find if I change

        this._container = DOM.create('details', 'maplibregl-ctrl maplibregl-ctrl-attrib mapboxgl-ctrl mapboxgl-ctrl-attrib');
        this._compactButton = DOM.create('summary', 'maplibregl-ctrl-attrib-button mapboxgl-ctrl-attrib-button', this._container);

to

        this._container = DOM.create('div', 'maplibregl-ctrl maplibregl-ctrl-attrib mapboxgl-ctrl mapboxgl-ctrl-attrib');
        this._compactButton = DOM.create('button', 'maplibregl-ctrl-attrib-button mapboxgl-ctrl-attrib-button', this._container);

it seems to work as expected.

This change is based on comparing the old html
<div class="maplibregl-ctrl maplibregl-ctrl-attrib mapboxgl-ctrl mapboxgl-ctrl-attrib maplibregl-compact mapboxgl-compact maplibregl-compact-show mapboxgl-compact-show"><button class="maplibregl-ctrl-attrib-button mapboxgl-ctrl-attrib-button" type="button" title="Toggle attribution" aria-label="Toggle attribution" aria-pressed="true"></button><div class="maplibregl-ctrl-attrib-inner mapboxgl-ctrl-attrib-inner" role="list"><a href="https://wifidb.net/" target="_blank">© WifiDB 2021-12-01</a> | <a href="https://www.eorc.jaxa.jp/ALOS/en/aw3d30/index.htm">AW3D30 (JAXA)</a> | <a href="https://wifidb.net/" target="_blank">© WifiDB</a> <a href="https://www.openmaptiles.org/" target="_blank">© OpenMapTiles</a> <a href="https://www.openstreetmap.org/copyright" target="_blank">© OpenStreetMap contributors</a> <a href="https://www.naturalearthdata.com/" target="_blank">Natural Earth</a></div></div>

to the new html
<details class="maplibregl-ctrl maplibregl-ctrl-attrib mapboxgl-ctrl mapboxgl-ctrl-attrib maplibregl-compact mapboxgl-compact maplibregl-compact-show mapboxgl-compact-show"><summary class="maplibregl-ctrl-attrib-button mapboxgl-ctrl-attrib-button" title="Toggle attribution" aria-label="Toggle attribution"></summary><div class="maplibregl-ctrl-attrib-inner mapboxgl-ctrl-attrib-inner"><a href="https://wifidb.net/" target="_blank">© WifiDB 2021-12-01</a> | <a href="https://www.eorc.jaxa.jp/ALOS/en/aw3d30/index.htm">AW3D30 (JAXA)</a> | <a href="https://wifidb.net/" target="_blank">© WifiDB</a> <a href="https://www.openmaptiles.org/" target="_blank">© OpenMapTiles</a> <a href="https://www.openstreetmap.org/copyright" target="_blank">© OpenStreetMap contributors</a> <a href="https://www.naturalearthdata.com/" target="_blank">Natural Earth</a></div></details>

you can see all the classes are there, it just wasn't displaying anything as a details/summary instead of div/button.

astridx · 2021-12-02T09:18:59Z

I am currently comparing the Attribution Control in the current developer version with version 1.15.2.
In 1.15.2, the control is displayed twice for screen sizes smaller than 640, although it should only be there once. Or am I missing something?
https://stackblitz.com/edit/web-platform-f7bvmv

astridx · 2021-12-02T09:57:50Z

@acalcutt Thank you very much for your support. You are right. I made the changes (div to details and button to summary) so that the control complies with a11y guidelines. Please see #557 and #359 (comment)

But you are also right. Of course, it has to work error-free. It now works for me with the changes in this PR with your example.

I added the unminified JavaScript file (Attribution control is on line 36794) to a fork of your stackblitz here:

https://stackblitz.com/edit/web-platform-jrpt1q?file=maplibre-gl.js

Is this working for you or am I missing something?

acalcutt · 2021-12-02T12:34:55Z

@acalcutt Thank you very much for your support. You are right. I made the changes (div to details and button to summary) so that the control complies with a11y guidelines. Please see #557 and #359 (comment)

But you are also right. Of course, it has to work error-free. It now works for me with the changes in this PR with your example.

I added the unminified JavaScript file (Attribution control is on line 36794) to a fork of your stackblitz here:

https://stackblitz.com/edit/web-platform-jrpt1q?file=maplibre-gl.js

Is this working for you or am I missing something?

Still not working. What this version does is slightly different. When it starts small on stackblitz, the button works as expected. If i then expand the window the button goes away and the attribution text shows (still good). However when I shrink the window again, the button comes back with a blank attribution

I am currently comparing the Attribution Control in the current developer version with version 1.15.2. In 1.15.2, the control is displayed twice for screen sizes smaller than 640, although it should only be there once. Or am I missing something? https://stackblitz.com/edit/web-platform-f7bvmv

The thing you missed is attribution shows by default unless you add "attributionControl: false," to the map options. So you are seeing the default one, plus the one you added. I only know this because I just moved my attribution to a different position, and when you do that you have to do the same thing or you get a double attribution button ( https://maplibre.org/maplibre-gl-js-docs/example/attribution-position/ )

      var map = new maplibregl.Map({
        container: "map",
        style: "https://demotiles.maplibre.org/style.json",
        center: [0.0, 0.0], // Startposition [lng, lat]
        zoom: 1, // Zoom
        attributionControl: false
      });

…o attr_fix_2

acalcutt · 2021-12-02T22:19:42Z

I looked over the code more and don't really see a reason details/summary works differently than the divs, but it does. Do you at least see what I mean in the video above when you try that with your example ( https://stackblitz.com/edit/web-platform-jrpt1q?file=maplibre-gl.js )

This is the same example with just details/summary put back to divs, which works fine for me
https://stackblitz.com/edit/web-platform-fre2ie?file=maplibre-gl.js

The logic for showing/hiding the button looks right to me...and the same code works with the divs so it is strange

astridx · 2021-12-02T22:35:43Z

I looked over the code more and don't really see a reason details/summary works differently than the divs, but it does.

The HTML elements details and summary automatically set the open attribute when the summary is clicked. As with the attribution control, the open/close is changed in some cases by resizing it works different

Do you at least see what I am in the video above when you try that with your example ( https://stackblitz.com/edit/web-platform-jrpt1q?file=maplibre-gl.js )

Yes, thank you for the explanation. Now I have seen it. I think it's fixed with the last commit, but I want to check tomorrow.

astridx · 2021-12-03T08:15:54Z

I have tested again and in my case compact==false, compact==true andcompact==undefined are correct. It would be great if someone would test again.

HarelM · 2021-12-03T13:28:36Z

I haven't tested the latest version, but here's what I see when I look at the code:
Upon initialization a compact flag is passed to the control - which causes a css class to be used.
This compact flag is not stored anywhere for future usage.
When a resize is called the compact flag is not taken into consideration but the same css classes are used and the only condition is around 640 pixels width (and not the initial compact flag that was passed).
To my understanding, and I might be wrong, the compact flag needs to be stored in the control class and be used when calling the _updateCompact method - this way the effect of resize to larger and then smaller than 640 pixel will not cause issues.
Feel free to correct my understanding of the code... :-)

acalcutt · 2021-12-03T15:07:46Z

I tested 92e230e and I though you had it fixed. In chrome it works as expected. But I was trying it in firefox and noticed one other strange issue that didn't happen in the old version

In firefox, when the window was small enough to show the button I clicked to expand it. I then resized the window just a little (not enough to show the full attribution as text, but a little bigger). When I did that the attribution text disappeared and I was back to that blank attribution button. if i toggled it, it stayed blank. It started working again if I made the window big enough to show text and then made it small again.

…mode, if it changed from big to small size

astridx · 2021-12-04T21:05:11Z

@HarelM To my understanding, and I might be wrong, the compact flag needs to be stored in the control class and be used when calling the _updateCompact method - this way the effect of resize to larger and then smaller than 640 pixel will not cause issues. Feel free to correct my understanding of the code... :-)

If I see this correctly, there are 3 possible states (not two: compact and not compact):
compact=true (the attribution control is always displayed compactly and per click the details are shown), compact=false (the attribution control is never displayed compactly) and compact = undefined (for wide displays the control is always displayed fully and for sizes < 640 pixels it is displayed compactly).

Only for compact = undefined is the_updateCompactmethod needed, which reacts to the change in screen width.

The HTML elements summary and details are optimised for displaying and hiding the details, the attribute open is set automatically, if summary is clicked. details is shown automatically if the attribute open is set - and vice versa.

Our variant for compact = undefined which is not handled by clicking but by changing the screen width is not covered here. It (the attribute open) has to be changed manually. I have done this here in PR. This is certainly not how the details and summary elements should actually be used. But I wanted to change as little as possible of the previous code.

I hope I have explained this in an understandable way.

If my current version is still incorrect, I would restore the original code (with div and button).

@acalcutt
Thank you for testing it so accurately. I have now corrected your last point of error. I hope that everything is now correct. Would you like to test again?

…o attr_fix_2

acalcutt · 2021-12-05T16:03:47Z

@astridx

I tested the latest version and it now works as I would expect. Thanks for taking the time to fix it and making it more compliant.

This reverts commit 0b81a41.

astridx · 2021-12-06T18:40:38Z

@acalcutt Thank you for testing.

a1272d9 b9b0370

This reverts commit 2031d8e.

This reverts commit 032b16e.

acalcutt · 2021-12-11T19:47:51Z

I got rid of the "compact" variable all together. It calls _updateCompact() when OnAdd is run, and also sets it to run _updateCompact() on a map resize event.

_updateCompact() already sets the classes and the open attribute the way it seems to need.

This is a clean compare of the changes I made, mixed with astridx's other fixes ( https://github.com/maplibre/maplibre-gl-js/compare/main...acalcutt:fix_attributes?expand=1 )

acalcutt · 2021-12-11T20:29:36Z

Here is a working example for testing https://stackblitz.com/edit/web-platform-wgdui8?file=maplibre-gl.js

HarelM · 2021-12-11T20:45:24Z

Comparing the code it seems that the following scenario will yield different results:
Setting compact = false and looking at the site from a device who's width is less than 640 pixels.
Let me know if I didn't get it right...

acalcutt · 2021-12-11T22:24:21Z

Well you did make me realize I was ignoring the compact option completely, so I also just added this change to UpdateCompact() to fix that (basically adding a check for the compact option, or a size <640).

    _updateCompact() {
        const compact = this.options && this.options.compact;
        if (this._map.getCanvasContainer().offsetWidth <= 640 || compact) {
            if (!this._container.classList.contains('maplibregl-compact')) {
                this._container.removeAttribute('open');
            }
            this._container.classList.add('maplibregl-compact', 'mapboxgl-compact');
        } else {
            this._container.setAttribute('open', '');
            this._container.classList.remove('maplibregl-compact', 'maplibregl-compact-show', 'mapboxgl-compact', 'mapboxgl-compact-show');
        }
    }

But to me that seems like it would affect compact = true not false. Reading the documentation, if compact = false suppsed to expand the button when below 640?

(options.compact) If true , force a compact attribution that shows the full attribution on mouse hover. If false , force the full attribution control. The default is a responsive attribution that collapses when the map is less than 640 pixels wide. Attribution should not be collapsed if it can comfortably fit on the map. compact should only be used to modify default attribution when map size makes it impossible to fit default attribution and when the automatic compact resizing for default settings are not sufficient .

acalcutt · 2021-12-12T02:30:33Z

I think I've fixed the issue with compact = false below 640. I made this update to _updateCompact() (https://github.com/maplibre/maplibre-gl-js/compare/main...acalcutt:fix_attributes?expand=1)

    _updateCompact() {
        const compact = this.options && this.options.compact;
        if (this._map.getCanvasContainer().offsetWidth <= 640 || compact) {
            if (compact === false) {
                this._container.setAttribute('open', '');
            } else {
                if (!this._container.classList.contains('maplibregl-compact')) {
                    this._container.removeAttribute('open');
                    this._container.classList.add('maplibregl-compact', 'mapboxgl-compact');
                }
            }
        } else {
            this._container.setAttribute('open', '');
            if (this._container.classList.contains('maplibregl-compact')) {
                this._container.classList.remove('maplibregl-compact', 'maplibregl-compact-show', 'mapboxgl-compact', 'mapboxgl-compact-show');
            }
        }
    }

I've updated the example here ( https://stackblitz.com/edit/web-platform-mw4nqr?file=index.html ) so the compact option can be set. It seems to work as I would expect with compact = true, compact = false, or compact not defined now

attribution_control.ts.txt

it was giving me a headache

HarelM · 2021-12-12T08:30:35Z

This looks good. We need to make sure we have the following tests to basically test all these scenarios right?
@astridx can you make sure all these tests now exists after this PR (I think they do looking at the code, but just to be sure):

Compact = true | false | undefined screen < 640
Compact = true | false | undefined screen > 640
Compact = true | false | undefined resize screen from <640 to >640 and back
So basically 9 use cases.
@acalcutt can you create a PR against this PR maybe so we can see if the tests can run against your code? or can you run the first version of what you suggested to see that the tests actually fail before your last fix of the compact = false?

…o attr_fix_2

…o acalcutt

astridx · 2021-12-14T13:29:33Z

Please see #713 (comment)

HarelM · 2021-12-14T13:49:32Z

I think the changes in the tests that are in this file should be changed to be order agnostic. Other than that, I think this PR is ready to be merged.
Thanks for all the hard work guys!!

astridx · 2021-12-14T13:55:12Z

I think the changes in the tests that are in this file should be changed to be order agnostic.

@HarelM That is now so, or am I missing something

HarelM

My bad, I read the changes incorrectly. Good job!!

* main: (98 commits) [Jest] Migrate `geojson_worker_source.test.js` (maplibre#731) Fix events being fired after Map#remove has been called when the WebGL context is lost and restored (maplibre#726) (maplibre#727) Define return type of getSource as possibly undefined (maplibre#724) Fix attibution controll (maplibre#668) Fix start-debug to watch ts files (maplibre#704) [Jest] Migrate `touch_zoom_rotate.test.js` (maplibre#721) [Jest] Migrate `requestRenderFrame.test.js` (maplibre#722) [Jest] Migrate `scroll_zoom.test.js` (maplibre#712) [Jest] Migrate `marker.test.js` (maplibre#696) [Jest] Migrate `mouse_rotate.test.js` (maplibre#711) [Jest] Migrate `keyboard.test.js` (maplibre#707) [Jest] Migrate `map_event.test.js` (maplibre#710) [Jest] Migrate `drag_rotate.test.js` (maplibre#709) Handle spies and call counts (maplibre#708) [Jest] Migrate `drag_pan.test.js` (maplibre#702) Add type for styleimagemissing event (maplibre#703) Fix MapDataEvent#isSourceLoaded being true in GeoJSONSource "dataloading" event handlers (maplibre#694) (maplibre#695) [Jest] Migrate `dblclick_zoom.test.js` (maplibre#697) [Jest] Migrate `popup.test.js` (maplibre#687) Fixed typo (maplibre#698) ...

* pluggable-render: (102 commits) WIP - pluggable render update Removed a badly typed variable [Jest] Migrate `geojson_worker_source.test.js` (maplibre#731) Fix events being fired after Map#remove has been called when the WebGL context is lost and restored (maplibre#726) (maplibre#727) Define return type of getSource as possibly undefined (maplibre#724) Fix attibution controll (maplibre#668) Fix start-debug to watch ts files (maplibre#704) Filter out hillshade layers Filter out hillshade styles [Jest] Migrate `touch_zoom_rotate.test.js` (maplibre#721) [Jest] Migrate `requestRenderFrame.test.js` (maplibre#722) [Jest] Migrate `scroll_zoom.test.js` (maplibre#712) [Jest] Migrate `marker.test.js` (maplibre#696) [Jest] Migrate `mouse_rotate.test.js` (maplibre#711) [Jest] Migrate `keyboard.test.js` (maplibre#707) [Jest] Migrate `map_event.test.js` (maplibre#710) [Jest] Migrate `drag_rotate.test.js` (maplibre#709) Handle spies and call counts (maplibre#708) [Jest] Migrate `drag_pan.test.js` (maplibre#702) Add type for styleimagemissing event (maplibre#703) ...

astridx added 2 commits November 30, 2021 13:27

fix attribution control

ebd0d4b

we do not need this

e4589f7

HarelM reviewed Nov 30, 2021

View reviewed changes

src/ui/control/attribution_control.ts Outdated Show resolved Hide resolved

acalcutt mentioned this pull request Dec 1, 2021

Attribution bug prozessor13/maplibre-gl-js#36

Closed

astridx added 2 commits December 1, 2021 20:22

Merge branch 'main' of https://github.com/maplibre/maplibre-gl-js int…

bd5219a

…o attr_fix_2

it is in 'compact = undefined'

bd1ee4b

Merge branch 'main' of https://github.com/maplibre/maplibre-gl-js int…

c1eb04c

…o attr_fix_2

work on attribute open on resize and compact = false

92e230e

astridx added 2 commits December 3, 2021 22:48

only remove the attribute open from container in compact===undefined …

a1272d9

…mode, if it changed from big to small size

add tests for last comit

b9b0370

Merge branch 'main' of https://github.com/maplibre/maplibre-gl-js int…

44e7fb0

…o attr_fix_2

acalcutt and others added 2 commits December 6, 2021 11:34

Update CHANGELOG.md

0b81a41

Revert "Update CHANGELOG.md"

2192c1d

This reverts commit 0b81a41.

acalcutt and others added 3 commits December 6, 2021 22:19

attribution fixes (from astridx)

2031d8e

a1272d9 b9b0370

Update .gitignore

032b16e

fix missing new line lint complained about

711436a

acalcutt mentioned this pull request Dec 7, 2021

Attribution not working at full screen #640

Closed

acalcutt added 5 commits December 11, 2021 13:47

Revert "attribution fixes (from astridx)"

53d97eb

This reverts commit 2031d8e.

Merge remote-tracking branch 'astridx/attr_fix_2' into fix_attributes

197c934

Try to simplify attribution control fix

7faee20

Revert "Update .gitignore"

6480b22

This reverts commit 032b16e.

fix "error Trailing spaces not allowed"

772381b

fix ignoring the compact option

e42e275

fix not being open below 640 when compact = false

b374fec

acalcutt added 3 commits December 11, 2021 22:25

add check if class exists before removing it

3494fc9

fix open being removed on resize

60e7b6b

un-reverse logic

c1ec082

it was giving me a headache

acalcutt mentioned this pull request Dec 12, 2021

Fix attributes #713

Closed

9 tasks

acalcutt and others added 5 commits December 12, 2021 13:13

fix class order to make tests pass

e0d86a3

Merge branch 'main' of https://github.com/maplibre/maplibre-gl-js int…

6d7c19b

…o attr_fix_2

Merge branch 'main' of https://github.com/maplibre/maplibre-gl-js int…

a2c09d8

…o acalcutt

change snapshot test to only check the attribute open

cf16f69

conflict

5176214

HarelM approved these changes Dec 14, 2021

View reviewed changes

HarelM merged commit 4930680 into maplibre:main Dec 21, 2021

astridx deleted the attr_fix_2 branch January 4, 2022 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix attibution controll #668

Fix attibution controll #668

astridx commented Nov 30, 2021 •

edited

Loading

github-actions bot commented Nov 30, 2021 •

edited

Loading

acalcutt commented Dec 2, 2021 •

edited

Loading

astridx commented Dec 2, 2021

astridx commented Dec 2, 2021

acalcutt commented Dec 2, 2021 •

edited

Loading

acalcutt commented Dec 2, 2021 •

edited

Loading

astridx commented Dec 2, 2021

astridx commented Dec 3, 2021

HarelM commented Dec 3, 2021

acalcutt commented Dec 3, 2021

astridx commented Dec 4, 2021 •

edited

Loading

acalcutt commented Dec 5, 2021

astridx commented Dec 6, 2021

acalcutt commented Dec 11, 2021

acalcutt commented Dec 11, 2021

HarelM commented Dec 11, 2021

acalcutt commented Dec 11, 2021

acalcutt commented Dec 12, 2021 •

edited

Loading

HarelM commented Dec 12, 2021

astridx commented Dec 14, 2021

HarelM commented Dec 14, 2021

astridx commented Dec 14, 2021

HarelM left a comment

Fix attibution controll #668

Fix attibution controll #668

Conversation

astridx commented Nov 30, 2021 • edited Loading

github-actions bot commented Nov 30, 2021 • edited Loading

acalcutt commented Dec 2, 2021 • edited Loading

astridx commented Dec 2, 2021

astridx commented Dec 2, 2021

acalcutt commented Dec 2, 2021 • edited Loading

acalcutt commented Dec 2, 2021 • edited Loading

astridx commented Dec 2, 2021

astridx commented Dec 3, 2021

HarelM commented Dec 3, 2021

acalcutt commented Dec 3, 2021

astridx commented Dec 4, 2021 • edited Loading

acalcutt commented Dec 5, 2021

astridx commented Dec 6, 2021

acalcutt commented Dec 11, 2021

acalcutt commented Dec 11, 2021

HarelM commented Dec 11, 2021

acalcutt commented Dec 11, 2021

acalcutt commented Dec 12, 2021 • edited Loading

HarelM commented Dec 12, 2021

astridx commented Dec 14, 2021

HarelM commented Dec 14, 2021

astridx commented Dec 14, 2021

HarelM left a comment

Choose a reason for hiding this comment

astridx commented Nov 30, 2021 •

edited

Loading

github-actions bot commented Nov 30, 2021 •

edited

Loading

acalcutt commented Dec 2, 2021 •

edited

Loading

acalcutt commented Dec 2, 2021 •

edited

Loading

acalcutt commented Dec 2, 2021 •

edited

Loading

astridx commented Dec 4, 2021 •

edited

Loading

acalcutt commented Dec 12, 2021 •

edited

Loading