Re: [PATCH yew-comp 1/3] task (viewer): add task download button

["Shannon Sterz" <s.sterz@proxmox.com>]

On Mon Mar 30, 2026 at 4:44 PM CEST, Dominik Csapak wrote:
>
>
> On 3/30/26 4:39 PM, Shannon Sterz wrote:
>> On Mon Mar 30, 2026 at 4:23 PM CEST, Dominik Csapak wrote:
>>>
>>>
>>> On 3/30/26 4:17 PM, Shannon Sterz wrote:
>>>> On Mon Mar 30, 2026 at 3:48 PM CEST, Dominik Csapak wrote:
>>>>>
>>>>>
>>>>> On 3/30/26 3:35 PM, Shannon Sterz wrote:
>>>>>> On Mon Mar 30, 2026 at 2:53 PM CEST, Dominik Csapak wrote:
>>>>> [snip]
>>>>>>> @@ -273,18 +297,94 @@ impl PwtTaskViewer {
>>>>>>>             let active = self.active;
>>>>>>>             let link = ctx.link();
>>>>>>>
>>>>>>> -        let toolbar = Toolbar::new().class("pwt-border-bottom").with_child(
>>>>>>> -            Button::new(tr!("Stop"))
>>>>>>> -                .disabled(!active)
>>>>>>> -                .onclick(link.callback(|_| Msg::StopTask)),
>>>>>>> -        );
>>>>>>> -
>>>>>>>             let url = format!(
>>>>>>>                 "{}/{}/log",
>>>>>>>                 props.base_url,
>>>>>>>                 percent_encode_component(&task_id),
>>>>>>>             );
>>>>>>>
>>>>>>> +        let filename = match props.task_id.parse::<ProxmoxUpid>() {
>>>>>>> +            Ok(upid) => {
>>>>>>> +                format!(
>>>>>>> +                    "task-{}-{}-{}.log",
>>>>>>> +                    upid.node,
>>>>>>> +                    upid.worker_type,
>>>>>>> +                    epoch_to_rfc3339_utc(upid.starttime).unwrap_or(upid.starttime.to_string())
>>>>>>> +                )
>>>>>>> +            }
>>>>>>> +            Err(_) => format!("task-{}.log", props.task_id),
>>>>>>> +        };
>>>>>>> +
>>>>>>> +        let download_btn: Html = if props.download_api {
>>>>>>> +            Container::from_tag("a")
>>>>>>> +                .attribute("disabled", active.then_some(""))
>>>>>>> +                .attribute("target", "_blank")
>>>>>>> +                .attribute("href", format!("/api2/extjs{url}?download=1"))
>>>>>>> +                .attribute("filename", filename)
>>>>>>> +                .with_child(
>>>>>>> +                    Button::new(tr!("Download"))
>>>>>>> +                        .disabled(active)
>>>>>>> +                        .icon_class("fa fa-download"),
>>>>>>> +                )
>>>>>>> +                .into()
>>>>>>
>>>>>> is there a reason you construct a new `<a>` element as a container here
>>>>>> instead of using the `download_as_file` helper in an `on_activate`
>>>>>> callback for the button?
>>>>>>
>>>>>
>>>>> if we have a static link, there is no need to create an 'a' element
>>>>> on the fly and clicking it programmatically. Since browsers use
>>>>> heuristics to detect 'user-initiated' actions to determine if things
>>>>> are allowed (e.g. auto-clicking on links) such helpers should be used
>>>>> sparingly.
>>>>>
>>>>> a static link is better expressed with native browser methods such as
>>>>> an a element that the user clicks. (the button inside is just for
>>>>> cosmetics in this case)
>>>>>
>>>>> e.g. if one would send a message in on_activate and call the helper in
>>>>> the update method, this might not work as intended since the browser
>>>>> can't detect that it was user-initiated.
>>>>
>>>> you could just call the helper directly instead of going through a
>>>> message though, no? however, im not sure if that is picked up as
>>>> user-initiated in all browsers either.
>>>>
>>>>> so my intent was to only use it when absolutely necessary.
>>>>>
>>>>> it's also what we do in PDM to download the report (though there we
>>>>> use a data url)
>>>>>
>>>>> the idea here was also that once all task log endpoints support the
>>>>> download api parameter, we can remove the button below again,
>>>>> and the helper too.
>>>>>
>>>>> do you see any advantage with using the helper in the first case too?
>>>>
>>>> my thinking was mostly about being able to re-use certain code paths
>>>> here.
>>>>
>>>> more importantly, though, from what i can tell the html 5 spec does not
>>>> allow interactive elements, like <button>, as descendants of <a>
>>>> elements:
>>>>
>>>>> Transparent, but there must be no interactive content descendant, a
>>>>> element descendant, or descendant with the tabindex attribute
>>>>> specified.
>>>>> - https://html.spec.whatwg.org/#the-a-element
>>>>
>>>> after looking into it some more, coding download buttons for static
>>>> resources as <a> elements is the right way to go accessibility-wise. so
>>>> maybe replacing the inner `Button` component with purely aesthetic
>>>> button.
>>>>
>>>> -->8 snip 8<--
>>>
>>> Sound good in general, I'll look into that, but since none of the part
>>> of the button is really interactive, i think it can be fine for now?
>>>
>>> (it's sadly not as simple as adding some class currently, since the e.g.
>>> ripple effect has to be done manually)
>>
>> i know it's a little trickier. sadly, this is definitively forbidden
>> regardless of "real interactivity". as per spec `<button>` is always
>> "interactive content":
>>
>>> § 3.2.5.2.7 Interactive content
>>>
>>> *Interactive content* is content that is specifically intended for
>>> user interaction.
>>>      => `a` (if the `href` attribute is present), `audio` (if the controls
>>>      attribute is present), `button`, `details`, `embed`, `iframe`,
>>>      `img` (if the usemap attribute is present), `input` (if the type
>>>      attribute is not in the Hidden state), `label`, `select`,
>>>      `textarea`, `video` (if the controls attribute is present)
>>> - https://html.spec.whatwg.org/#interactive-content-2
>>
>> we also set a tabindex attribute on buttons unconditionally [1], this is
>> also explicitly forbidden for descendants of the `<a>` element.
>>
>> [1]: https://git.proxmox.com/?p=ui/proxmox-yew-widget-toolkit.git;a=blob;f=src/widget/button.rs;h=584f923c;hb=HEAD#l289
>
>
> yes it might be "forbidden", but still works for these purposes for now
> until we have a better way? Not sure if we want to block features
> just to be spec compliant when it clearly works in all relevant
> browsers, especially when I already said I'll fix it.

sorry if i misunderstood you here, i'm not saying this should be a
general blocker. i was just concerned that invalid markup might mess
with certain screen readers and other accessibility aids.

(added the list back as i accidentally dropped it along the way, sorry
for that)