Narrow the role denial to actions: keep the commit-page link live

The previous two commits disabled every commit affordance on a role-denied
package, including "Create new revision" — which is a link, not a button.
Supporting that meant inventing disabled-link semantics on a shared
component: dropping the href, setting aria-disabled, and extending the
stylesheet, none of which anything else needed.

The rule is now: disable actions that cannot succeed, never navigation.
Navigating to the commit page is harmless and useful — that page opens under
a denial, shows the banner, and disables its own commit buttons with a
tooltip, so the user can still read their changes, review the file list and
write a message. The explanation belongs at the point of action, not one
screen earlier. It also makes the packages list consistent rather than
exceptional: it hides CommitAndPush, an action, and touches nothing
navigational.

So CreateNewRevision goes back to a plain link, and everything that existed
only to disable it is reverted to its pre-branch state: the `disabled` prop
on CtaLink, its pass-through in CreateNewRevision, and the
[aria-disabled="true"] rules in button.css. Nothing else had picked any of
them up. CommitAndPush on the detail page stays disabled with its tooltip,
and the commit page is untouched.

The page's tests keep the denied-vs-readable contrast and gain one pinning
the new rule: the action bar's markup is sliced out of the page source and
asserted to render the commit-page link plain — no `disabled`, no popover
wrapper — while the action after it still carries its denial tooltip. A host
-target test cannot render the Leptos view, and a boolean seam for a value
that is always false would test a fiction rather than the markup someone
would edit while "fixing the inconsistency".

Author: fiskus

quilt-sync/CHANGELOG.md

@@ -13,7 +13,7 @@
 
 ### Changed
 
-- The package page and the commit page now disable Commit and Commit-and-Push when the active role cannot read the package's bucket, explaining in a tooltip that names the role and points at switching it, instead of letting the commit fail with a storage error (<https://github.com/quiltdata/quilt-rs/pull/NNN>)
+- The commit buttons on the package page and the commit page are now disabled when the active role cannot read the package's bucket, explaining in a tooltip that names the role and points at switching it, instead of letting the commit fail with a storage error; opening the commit page and reviewing your changes there still works (<https://github.com/quiltdata/quilt-rs/pull/NNN>)
 
 ## [v0.20.0-alpha2] - 2026-07-28
 

quilt-sync/ui/assets/css/components/button.css

@@ -20,16 +20,12 @@
     box-shadow: var(--q-ui-shadows-button-hover);
   }
 
-  /* An anchor has no `disabled` attribute, so an inert link (see `CtaLink`)
-     states itself with `aria-disabled` and gets the same treatment. */
-  &[disabled],
-  &[aria-disabled="true"] {
+  &[disabled] {
     opacity: 0.7;
     cursor: not-allowed;
   }
 
-  &[disabled]:hover,
-  &[aria-disabled="true"]:hover {
+  &[disabled]:hover {
     background-color: #fff;
     box-shadow: var(--q-ui-shadows-button);
   }
@@ -39,8 +35,7 @@
     background: var(--q-ui-palette-primary);
   }
 
-  &.primary[disabled]:hover,
-  &.primary[aria-disabled="true"]:hover {
+  &.primary[disabled]:hover {
     color: #fff;
     background: var(--q-ui-palette-primary);
   }

quilt-sync/ui/src/components/buttons/base/cta.rs

@@ -35,23 +35,13 @@ pub fn CtaLink(
     href: String,
     #[prop(optional)] icon: Option<&'static str>,
     #[prop(optional, into)] primary: MaybeProp<bool>,
-    /// Render the link inert. An anchor is not a form control, so there is no
-    /// `disabled` attribute to set: instead the `href` is dropped — an anchor
-    /// without one is neither a hyperlink nor keyboard-focusable — and
-    /// `aria-disabled` says the same to assistive tech. The stylesheet keys
-    /// the dimmed, not-allowed treatment off `[aria-disabled="true"]`
-    /// alongside the `[disabled]` it already applies to buttons.
-    #[prop(optional, into)]
-    disabled: MaybeProp<bool>,
     children: Children,
 ) -> impl IntoView {
-    let is_disabled = Signal::derive(move || disabled.get().unwrap_or(false));
     view! {
         <a
             class="qui-button large"
             class:primary=move || primary.get().unwrap_or(false)
-            href=move || (!is_disabled.get()).then(|| href.clone())
-            aria-disabled=move || is_disabled.get().then_some("true")
+            href=href
         >
             <span>{children()}</span>
             {icon.map(|src| view! { <img class="qui-icon" src=src /> })}

quilt-sync/ui/src/components/buttons/create_new_revision.rs

@@ -8,10 +8,9 @@ const KIND: ButtonKind = ButtonKind::CreateNewRevision;
 pub fn CreateNewRevision(
     href: String,
     #[prop(optional, into)] primary: MaybeProp<bool>,
-    #[prop(optional, into)] disabled: MaybeProp<bool>,
 ) -> impl IntoView {
     view! {
-        <CtaLink href=href icon=KIND.icon() primary=primary disabled=disabled>
+        <CtaLink href=href icon=KIND.icon() primary=primary>
             {KIND.label()}
         </CtaLink>
     }

quilt-sync/ui/src/pages/installed_package/content.rs

@@ -199,10 +199,11 @@ pub(super) fn InstalledPackageContent(
     // Push is offered only when there's a remote and something to ship.
     let is_publishable = has_origin
         && (status == "ahead" || (status == "up_to_date" && has_changes) || status == "local");
-    // A role denial disables both action-bar affordances (see
-    // `commit_affordance_disabled`) and gives them a shared tooltip. Disabled,
-    // not hidden: the user opened this page deliberately, and a button that
-    // simply vanishes explains nothing.
+    // A role denial disables the action bar's *action* — Commit and Push —
+    // and gives it a tooltip. Disabled, not hidden: the user opened this page
+    // deliberately, and a button that simply vanishes explains nothing.
+    // "Create new revision" is navigation and stays live (see
+    // `commit_affordance_disabled`).
     let commit_denied = commit_affordance_disabled(no_access_reason.as_deref());
     let commit_hint = util::commit_denied_hint(no_access_reason.as_deref());
 
@@ -374,21 +375,11 @@ pub(super) fn InstalledPackageContent(
                 );
                 view! {
                     <div class="qui-actionbar">
-                        {with_popover(
-                            commit_hint.clone(),
-                            view! {
-                                <buttons::CreateNewRevision
-                                    href=href
-                                    primary=revision_primary
-                                    disabled=commit_denied
-                                />
-                            }
-                                .into_any(),
-                        )}
+                        <buttons::CreateNewRevision href=href primary=revision_primary />
                         {is_publishable.then(|| view! {
                             <span class="actions-divider">"or"</span>
                             {with_popover(
-                                commit_hint.clone(),
+                                commit_hint,
                                 view! {
                                     <buttons::CommitAndPush
                                         on_click=on_publish
@@ -445,16 +436,21 @@ pub(super) fn InstalledPackageContent(
     }
 }
 
-/// Whether this page's commit affordances — "Create new revision", which is
-/// the way into the commit page, and "Commit and Push" — must be inert.
+/// Whether this page's commit *action* — "Commit and Push" — must be inert.
 ///
 /// Committing looks like offline work but is not: the workflow quality gate
 /// reads the bucket's `.quilt/workflows/config.yml` before any manifest is
 /// written, so under a role that cannot read the bucket every commit is a 403.
-/// The packages *list* hides its affordance instead — a list row carries its
-/// own visible denial reason, and an inert button there would be noise — but a
-/// page the user navigated to must disable and explain, which is what
-/// [`crate::util::commit_denied_hint`] supplies.
+/// A page the user navigated to disables and explains rather than hiding,
+/// which is what [`crate::util::commit_denied_hint`] supplies. (The packages
+/// *list* hides its affordance instead: a list row carries its own visible
+/// denial reason, and an inert button there would be noise.)
+///
+/// Deliberately narrow: this gates actions that cannot succeed, never
+/// navigation. "Create new revision" is a link to the commit page, and that
+/// page opens fine under a denial — it shows the banner and disables its own
+/// commit buttons — so the user can still read their changes and the
+/// explanation lands at the point of action instead of one screen earlier.
 ///
 /// A pure seam so the contrast (denied disables, readable does not) is
 /// testable without a DOM, matching `publish_affordance` on the list page.
@@ -485,23 +481,71 @@ mod tests {
 
     const DENIED: &str = "Current role ReadOnly has no access to this bucket";
 
-    /// A denied bucket makes the page's commit affordances inert, and — the
+    /// The page's own source, so the markup rule below can be checked without
+    /// a DOM: these tests run on the host target, where the Leptos view is
+    /// never rendered.
+    const SOURCE: &str = include_str!("content.rs");
+
+    /// A denied bucket makes the page's commit *action* inert, and — the
     /// contrast that keeps this test honest — an otherwise identical package
-    /// on a readable bucket leaves them live.
+    /// on a readable bucket leaves it live.
     #[test]
-    fn a_denied_bucket_disables_the_commit_affordances_a_readable_one_does_not() {
+    fn a_denied_bucket_disables_the_commit_action_a_readable_one_does_not() {
         assert!(commit_affordance_disabled(Some(DENIED)));
         assert!(!commit_affordance_disabled(None));
     }
 
-    /// The disabled affordances are not silent: the same denial supplies the
+    /// The rule is "disable actions that cannot succeed, never navigation".
+    /// "Create new revision" is a link to the commit page, which opens fine
+    /// under a denial and explains itself there, so it must stay a plain,
+    /// followable link — no `disabled`, no tooltip wrapper pre-empting it one
+    /// screen early. Pinned against the markup because that is where someone
+    /// would undo it while "fixing the inconsistency" with Commit and Push.
+    #[test]
+    fn the_link_to_the_commit_page_is_never_disabled() {
+        let bar = action_bar_markup();
+        let start = bar
+            .find("<buttons::CreateNewRevision")
+            .expect("the action bar still renders the commit-page link");
+        let (before, from_link) = bar.split_at(start);
+        let element = &from_link[..from_link
+            .find("/>")
+            .expect("the link element is self-closing")];
+
+        assert!(
+            !element.contains("disabled"),
+            "the commit-page link must stay followable, got: {element}"
+        );
+        assert!(
+            !before.contains("with_popover"),
+            "the commit-page link must not be wrapped in a denial tooltip"
+        );
+        // The contrast: the action *after* it does carry one, so this test
+        // cannot pass merely because every tooltip was deleted.
+        assert!(
+            from_link.contains("with_popover"),
+            "Commit and Push still explains its denial"
+        );
+    }
+
+    /// The disabled action is not silent: the same denial supplies the
     /// tooltip naming the role and the fix, and a readable bucket has none.
     #[test]
-    fn the_disabled_commit_affordances_explain_themselves() {
+    fn the_disabled_commit_action_explains_itself() {
         assert_eq!(
             commit_denied_hint(Some(DENIED)).as_deref(),
             Some("Current role ReadOnly has no access to this bucket. Switch role to commit.")
         );
         assert_eq!(commit_denied_hint(None), None);
     }
+
+    /// The action bar's markup, sliced out of [`SOURCE`] so the assertions
+    /// above never read this test module's own text.
+    fn action_bar_markup() -> &'static str {
+        let start = SOURCE
+            .find(r#"<div class="qui-actionbar">"#)
+            .expect("the page still renders an action bar");
+        let bar = &SOURCE[start..];
+        &bar[..bar.find("</div>").expect("the action bar is closed")]
+    }
 }