Fix attributes #713

acalcutt · 2021-12-12T14:39:21Z

This PR is meant to fix the issues with attribution mentioned here ( #640 (comment) ) . It is a continuation of work done by astridx here ( #668 ) with some changes to make it a little more readable.

The test included in this were made by astridx and I am not familiar with testing them. I assume this is part or test-render, but I gt WebGL error when i try to run those tests on my headless server I use to build.

I have manually tested the various states of the compact option and the issues we found while resizing the window and they seem to work for me.

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
Document any changes to public APIs.
Post benchmark scores.
Manually test the debug page.
Suggest a changelog category: bug/feature/docs/etc. or "skip changelog".
Add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>.

…o attr_fix_2

…mode, if it changed from big to small size

This reverts commit 0b81a41.

maplibre@a1272d9 maplibre@b9b0370

This reverts commit 2031d8e.

This reverts commit 032b16e.

it was giving me a headache

wipfli · 2021-12-12T14:45:09Z

WebGL error when i try to run those tests on my headless server I use to build.

Can you try xvfb-run -s "-ac -screen 0 1280x1024x24" npm run test-unit? See https://github.com/stackgl/headless-gl#how-can-headless-gl-be-used-on-a-headless-linux-machine and the linux workflows:

maplibre-gl-js/.github/workflows/test-unit.yml

Lines 35 to 49 in d00f754

    
           test_unit_ubuntu: 
        
             name: ubuntu-latest 
        
             runs-on: ubuntu-latest 
        
             timeout-minutes: 20 
        
             steps: 
        
               - uses: actions/checkout@v2 
        
               - name: Use Node.js 14 x64 
        
                 uses: actions/setup-node@v2 
        
                 with: 
        
                   node-version: 14 
        
                   architecture: x64 
        
               - run: npm ci 
        
               - run: npm run build-dev 
        
               # see: https://github.com/stackgl/headless-gl#how-can-headless-gl-be-used-on-a-headless-linux-machine 
        
               - run: xvfb-run -s "-ac -screen 0 1280x1024x24" npm run test-unit

wipfli · 2021-12-12T14:45:58Z

If that works for you, feel free to update the contribution guidelines and submit a pull request.

github-actions · 2021-12-12T14:48:24Z

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

HarelM · 2021-12-12T15:42:41Z

Interestingly enough, the tests fail :-) this is the final version of your changes, right?

acalcutt · 2021-12-12T16:51:44Z

Yes, it seems to be the latest version.

Is it saying it failed on this test?
Snapshot name: AttributionControl Snapshot Tests Details is set correct for default view (compact === undefined) The attribute open="" should NOT change on resize from <= 640 to another <= 640 if it is open. 3

That should work since it only removes that open attribute when the classes don't contain maplibregl-compact

Line 164

                if (!this._container.classList.contains('maplibregl-compact')) {
                    this._container.removeAttribute('open');
                    this._container.classList.add('maplibregl-compact', 'mapboxgl-compact');
                }

In my testing that seems to work. the open attribute stays on resize below 640.

acalcutt · 2021-12-12T16:56:15Z

WebGL error when i try to run those tests on my headless server I use to build.

Can you try xvfb-run -s "-ac -screen 0 1280x1024x24" npm run test-unit? See https://github.com/stackgl/headless-gl#how-can-headless-gl-be-used-on-a-headless-linux-machine and the linux workflows:

maplibre-gl-js/.github/workflows/test-unit.yml

Lines 35 to 49 in d00f754

test_unit_ubuntu:

name: ubuntu-latest

runs-on: ubuntu-latest

timeout-minutes: 20

steps:

- uses: actions/checkout@v2

- name: Use Node.js 14 x64

uses: actions/setup-node@v2

with:

node-version: 14

architecture: x64

- run: npm ci

- run: npm run build-dev

# see: https://github.com/stackgl/headless-gl#how-can-headless-gl-be-used-on-a-headless-linux-machine

- run: xvfb-run -s "-ac -screen 0 1280x1024x24" npm run test-unit

I tried this but I still get many errors that seem to be related to WebGL not being able to initialize
log.txt

acalcutt · 2021-12-12T18:24:25Z

I think I fixed the tests in attribution_control.test.ts . It looks like it was complaining about the class order not matching the Inline Snapshot. In all the tests that failed the classes matched, but they were in a slightly different order.

I was able to set up an environment in windows and tested it there


PS C:\Users\Andrew\Documents\test\fix_attributes> npm run test-jest

> [email protected] test-jest C:\Users\Andrew\Documents\test\fix_attributes
> jest

 PASS  src/ui/camera.test.ts (25.684 s)
 PASS  src/ui/popup.test.ts (27.026 s)
 PASS  src/source/vector_tile_source.test.ts
 PASS  src/style-spec/style-spec.test.ts
 PASS  src/ui/handler/dblclick_zoom.test.ts
 PASS  src/ui/map/isMoving.test.ts
 PASS  src/ui/control/attribution_control.test.ts (33.231 s)
 PASS  src/ui/map_events.test.ts
 PASS  src/ui/map/isZooming.test.ts
 PASS  src/ui/control/logo_control.test.ts
 PASS  src/ui/control/geolocate_control.test.ts
 PASS  src/geo/edge_insets.test.ts
 PASS  src/ui/hash.test.ts
 PASS  src/style-spec/feature_filter/feature_filter.test.ts
 PASS  src/ui/control/scale_control.test.ts
 PASS  src/index.test.ts
 PASS  src/ui/map/isRotating.test.ts
 PASS  src/ui/control/fullscreen_control.test.ts
 PASS  src/source/video_source.test.ts
 PASS  src/ui/handler/box_zoom.test.ts
 PASS  src/style/style_layer.test.ts
 PASS  src/data/bucket/line_bucket.test.ts
 PASS  src/symbol/shaping.test.ts
 PASS  src/source/tile.test.ts
 PASS  src/util/worker_pool.test.ts
 PASS  src/source/worker.test.ts
 PASS  src/style-spec/function/index.test.ts
 PASS  src/data/bucket/symbol_bucket.test.ts
 PASS  src/source/tile_id.test.ts
 PASS  src/symbol/symbol_style_layer.test.ts
 PASS  src/geo/lng_lat_bounds.test.ts
 PASS  src/render/glyph_manager.test.ts
 PASS  src/geo/transform.test.ts
 PASS  src/util/color_ramp.test.ts
 PASS  src/data/bucket/fill_bucket.test.ts
 PASS  src/util/struct_array.test.ts
 PASS  src/data/load_geometry.test.ts
 PASS  src/source/canvas_source.test.ts
 PASS  src/data/feature_position_map.test.ts
 PASS  src/style/style_layer_index.test.ts
 PASS  src/source/query_features.test.ts
 PASS  src/style-spec/migrate/v8.test.ts
 PASS  src/symbol/get_anchors.test.ts
 PASS  src/symbol/quads.test.ts
 PASS  src/data/dem_data.test.ts
 PASS  src/style-spec/expression/expression.test.ts
 PASS  src/style-spec/empty.test.ts
 PASS  src/style/format_section_override.test.ts
 PASS  src/gl/state.test.ts
 PASS  src/style-spec/migrate/v9.test.ts
 PASS  src/style-spec/composite.test.ts
 PASS  src/symbol/check_max_angle.test.ts
 PASS  src/util/throttle.test.ts
 PASS  src/symbol/path_interpolator.test.ts
 PASS  src/style/style_layer/fill_style_layer.test.ts
 PASS  src/symbol/clip_line.test.ts
 PASS  src/gl/vertex_buffer.test.ts
 PASS  src/style/light.test.ts
 PASS  src/util/request_manager.test.ts
 PASS  src/util/evented.test.ts
 PASS  src/style-spec/util/color.test.ts
 PASS  src/style-spec/util/interpolate.test.ts
 PASS  src/symbol/cross_tile_symbol_index.test.ts
 PASS  src/util/task_queue.test.ts
 PASS  src/symbol/grid_index.test.ts
 PASS  src/util/browser.test.ts
 PASS  src/style-spec/group_by_layout.test.ts
 PASS  src/shaders/encode_attribute.test.ts
 PASS  src/source/tile_cache.test.ts
 PASS  src/symbol/mergelines.test.ts
 PASS  src/source/geojson_wrapper.test.ts
 PASS  src/geo/lng_lat.test.ts
 PASS  src/util/classify_rings.test.ts
 PASS  src/util/primitives.test.ts
 PASS  src/util/find_pole_of_inaccessibility.test.ts
 PASS  src/util/resolve_tokens.test.ts
 PASS  src/symbol/collision_feature.test.ts
 PASS  src/render/uniform_binding.test.ts
 PASS  src/style-spec/diff.test.ts
 PASS  src/render/line_atlas.test.ts
 PASS  src/symbol/anchor.test.ts
 PASS  src/style-spec/util/color_spaces.test.ts
 PASS  src/style-spec/expression/stops.test.ts
 PASS  src/geo/mercator_coordinate.test.ts
 PASS  src/style-spec/format.test.ts
 PASS  src/style-spec/declass.test.ts
 PASS  src/style-spec/deref.test.ts

Test Suites: 87 passed, 87 total
Tests:       967 passed, 967 total
Snapshots:   22 passed, 22 total
Time:        51.641 s
Ran all test suites.

HarelM · 2021-12-12T18:33:57Z

Cool!! The tests pass also in this CI, congrats :-)
I tend to say that class order should not make a test fail. It might be good to change the tests to be class-order-independent...?
@astridx what do you think? Which code do you think is more readable?

astridx · 2021-12-14T13:27:48Z

@acalcutt
I am sorry that my reply is late. I am glad that you are helping me. That way I can learn :)

@HarelM
I also had a bad feeling about the snapshot tests. You always read that GUI can be tested well with snapshots. But because we are only dealing with the open attribute here, this variant is not ideal. I have changed it in my PR #668.

In addition, I have integrated @acalcutt changes into my PR. Is that OK ? The git history shows that @acalcutt made the changes. Otherwise my PR can be closed and the changes of the tests can be integrated in another PR #668, if you like them.

acalcutt · 2021-12-14T13:29:49Z

Looking at the code the tests were originally made for ( https://github.com/maplibre/maplibre-gl-js/blob/a1272d9f569057fed6ce3b163fc0602f824e7653/src/ui/control/attribution_control.ts ), I'm pretty sure the classes just changed order because in that version _updateAttributions(); is called before _updateCompact(); , where in my version I put _updateCompact() before _updateAttributions().

I don't think the class order will change again unless the order of those functions changes.

acalcutt · 2021-12-14T13:32:12Z

@HarelM @astridx I am happy to have my changes included as part of either PR. so feel free to include them.

HarelM · 2021-12-14T13:51:03Z

Since both PRs now have (almost?) the same code I think we should close this one in favor of #668

acalcutt · 2021-12-14T14:36:40Z

Closed, since #668 has these updates + test fixes

astridx and others added 26 commits November 30, 2021 13:27

fix attribution control

ebd0d4b

we do not need this

e4589f7

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

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

Update CHANGELOG.md

0b81a41

Revert "Update CHANGELOG.md"

2192c1d

This reverts commit 0b81a41.

attribution fixes (from astridx)

2031d8e

maplibre@a1272d9 maplibre@b9b0370

Update .gitignore

032b16e

fix missing new line lint complained about

711436a

Merge branch 'maplibre:main' into main

41be4b5

Merge branch 'maplibre:main' into main

629e818

Merge branch 'maplibre:main' into main

c68a94c

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

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

fix class order to make tests pass

e0d86a3

astridx mentioned this pull request Dec 14, 2021

Fix attibution controll #668

Merged

acalcutt closed this Dec 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix attributes #713

Fix attributes #713

acalcutt commented Dec 12, 2021

wipfli commented Dec 12, 2021

wipfli commented Dec 12, 2021

github-actions bot commented Dec 12, 2021 •

edited

Loading

HarelM commented Dec 12, 2021

acalcutt commented Dec 12, 2021

acalcutt commented Dec 12, 2021

acalcutt commented Dec 12, 2021

HarelM commented Dec 12, 2021

astridx commented Dec 14, 2021 •

edited

Loading

acalcutt commented Dec 14, 2021

acalcutt commented Dec 14, 2021

HarelM commented Dec 14, 2021

acalcutt commented Dec 14, 2021

Fix attributes #713

Fix attributes #713

Conversation

acalcutt commented Dec 12, 2021

wipfli commented Dec 12, 2021

wipfli commented Dec 12, 2021

github-actions bot commented Dec 12, 2021 • edited Loading

HarelM commented Dec 12, 2021

acalcutt commented Dec 12, 2021

acalcutt commented Dec 12, 2021

acalcutt commented Dec 12, 2021

HarelM commented Dec 12, 2021

astridx commented Dec 14, 2021 • edited Loading

acalcutt commented Dec 14, 2021

acalcutt commented Dec 14, 2021

HarelM commented Dec 14, 2021

acalcutt commented Dec 14, 2021

github-actions bot commented Dec 12, 2021 •

edited

Loading

astridx commented Dec 14, 2021 •

edited

Loading