Generated with the help of an LLM.

Problem/Motivation

Access checks on Mapillary content in organizations (OrganizationAccess) work one entity at a time:

  • organizations() loads the relationships of the entity. Group caches them per entity for the request, so a page of 50 rows still loads them 50 times.
  • governingOrganizations() runs a group query on every call. For a sequence, a map feature or a detection, OrganizationGovernance::mapillaryOrganizationIds() also runs one query per path through its images.
  • The update and delete checks of AccessHooks::entityAccess() call the view check first, so a row with edit and delete operations repeats the governing lookup.
  • On a cache miss, the notices on the organization page and the "Mismatched content" tab access-check every candidate image.

The listing queries are filtered in the database; the cost is in the per-row checks and grows with the rows of a page.

Left over from #3633409: Clean up the deferred findings of Mapillary organizations on Group and #3634056: Improve screen reader, keyboard focus, reduced motion and narrow screen support in the map picker and the split viewer:

  • CHANGELOG.md has no entries for #3633409: Clean up the deferred findings of Mapillary organizations on Group. AGENTS.md still describes the organization filter as a numeric group ID text field and says that hooks check moduleExists('group'); both changed there. README.md does not say that the page of a public organization is open to non-members.
  • #3634056: Improve screen reader, keyboard focus, reduced motion and narrow screen support in the map picker and the split viewer was merged without the cluster item: with reduced motion, the picker's clusterGroup() still uses the animations of markercluster. Drupal.mapillary.prefersReducedMotion() is in js/mapillary-viewer.js (mapillary/viewer), and the picker library mapillary_leaflet/picker does not depend on it.
  • The focus goes back to a marker after a redraw, but not to a cluster: a zoom that splits or merges a cluster replaces its element.
  • For the .mapillary-split-marker__cone rule in mapillary-split-viewer.css, stylelint with core's configuration wants the radial-gradient() value wrapped and the Prettier CLI wants it on one line.

Steps to reproduce

  1. Enable mapillary_organization, create a members-only organization with a Mapillary organization ID and import 50 images of that Mapillary organization.
  2. Log the database queries and open the image listing as a member with the edit permission: the number of queries grows with the number of rows.
  3. Turn on "Reduce motion" in the operating system, open the map picker and zoom out until markers cluster: the clusters still animate.
  4. Give a cluster the keyboard focus and zoom the map until it splits: the focus leaves the map.

Proposed resolution

Access (mapillary_organization):

  • organizations() and governingOrganizations() keep their answer per entity for the request until a tag it depends on is invalidated, as organizationIdsByViewAccess() does.
  • A hook_entity_load() implementation preloads the organizations of the Mapillary images, sequences, map features and detections being loaded: one relationship query per entity type, and for sequences, map features and detections one query per governance path.
  • The members-only organizations by Mapillary organization ID are read once per request.
  • Who may see and edit what does not change; the existing access tests stay as they are. A kernel test counts the queries of view, update and delete checks on 1 and on 20 entities of each type and asserts that the count does not grow with the entities.

Map picker and split viewer:

  • Drupal.mapillary.prefersReducedMotion() moves to a small library of the base module, mapillary/motion, which mapillary/viewer and mapillary_leaflet/picker depend on. With reduced motion, the picker's cluster groups are created with animate: false.
  • When a redraw replaces the cluster that has the focus, the focus goes to the marker or cluster nearest to its position, or to the map when there is none.
  • The cone rule is written the way stylelint wants it, since Drupal CI runs stylelint.
  • Tests: MapillaryPickerTest checks the cluster focus after a zoom and, with matchMedia stubbed, that the cluster groups are created without animation.

Documentation:

  • CHANGELOG.md: the changes of #3633409: Clean up the deferred findings of Mapillary organizations on Group that site builders and users see, and the entries of this issue, under [Unreleased].
  • AGENTS.md: the two outdated lines, the preloading and the mapillary/motion library.
  • README.md: the page of a public organization is open to non-members; the map picker follows reduced motion.

Upgrade path: no schema or configuration changes. An empty post update function rebuilds the container for the new hook implementation.

Remaining tasks

  • Implementation and tests.
  • Merge request.
  • Review.

User interface changes

  • With reduced motion, map picker clusters do not animate.
  • The keyboard focus stays on the map when a focused cluster splits or merges.

API changes

  • New library mapillary/motion. Drupal.mapillary.prefersReducedMotion() keeps its name; mapillary/viewer depends on the new library, so code that uses the helper through mapillary/viewer keeps working.

Data model changes

None.

Issue fork mapillary-3634264

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

orkutmuratyilmaz created an issue. See original summary.

  • orkutmuratyilmaz committed 03045cec on 1.0.x
    docs: #3634264: Describe the preloaded access checks, the cluster focus...

  • orkutmuratyilmaz committed c227dd63 on 1.0.x
    fix: #3634264: Keep the focus on the map when a focused cluster of the...

  • orkutmuratyilmaz committed 19de92b9 on 1.0.x
    fix: #3634264: Wrap the gradient of the viewing cone the way stylelint...

  • orkutmuratyilmaz committed 1f42388f on 1.0.x
    docs: #3634264: Add the entries of #3633409 to the changelog and correct...
orkutmuratyilmaz’s picture

Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.