1257608 - Change attachment button into attachment link

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: User Interface, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

The normal bugzilla UI uses a link to attach files, so you can ctrl-click it to open in new tab.  The experimental interface uses a button, forcing it to clobber your tab.

Comment 1

[David Lawrence [:dkl]]

Attachment: 1257608_1.patch

Comment 2

[:glob ✱]

Comment on attachment 8731860 [details] [diff] [review]
1257608_1.patch

Review of attachment 8731860 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/BugModal/web/bug_modal.css
@@ +44,4 @@
>      padding: 4px 8px;
>  }
>  
> +button.minor, #attachments-add-btn {

selection should be done by class, not by id.
eg. apply a 'button-minor' class to the anchor and select on a.button-minor

Comment 3

[David Lawrence [:dkl]]

Attachment: 1257608_2.patch

Comment 5

[:glob ✱]

Comment on attachment 8731901 [details] [diff] [review]
1257608_2.patch

Review of attachment 8731901 [details] [diff] [review]:
-----------------------------------------------------------------

this change results in the "attach file" link being rendered differently from other buttons, so it looks out of place.
differences include:
- different font
- different height
- no border-radius
- transition animation missing on :hover

as buttons are rendered by the system, it's possible that heights/radii are different between platforms; please check if this is the case.
fixing this may require changing all buttons in the UI to links.

::: extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl
@@ +1089,4 @@
>  
>  [% IF user.id %]
>    <div id="top-actions">
> +    <a id="attachments-add-btn" class="button-minor" href="attachment.cgi?bugid=[% bug.bug_id FILTER uri %]&action=enter">Attach File</a>

& should be &amp;

Comment 6

[:glob ✱]

Attachment: different buttons

Comment 7

[Sebastin Santy [:seban]]

Attachment: rev1

Fixed some nits that were left out.

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 8773433 [details] [review]
rev1

You cannot nest a <button> inside an <a> tag or it will not pass HTML5 validation.

https://stackoverflow.com/questions/6393827/can-i-nest-a-button-element-inside-an-a-using-html5

I am almost done with a new patch for this which makes certain <a> tags look like buttons and gives the fix we need.

dkl

Comment 9

[David Lawrence [:dkl]]

Attachment: 1257608_3.patch

Comment 10

[David Lawrence [:dkl]]

Attachment: 1257608_4.patch

Remove some changes to comments.js about updating the tags menu which is done as separate bug 1262457.

dkl

Comment 12

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8815811 [details] [diff] [review]
1257608_4.patch

functionality works, and it doesn't break with CSP turned on. There are some cosmetic defects I think should be fixed, I'm attaching a screenshot.

Comment 13

[Dylan Hardison [:dylan] (he/him)]

Attachment: output.png

Comment 14

[David Lawrence [:dkl]]

Attachment: 1257608_5.patch

Cosmetic fixes.

Comment 15

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8829659 [details] [diff] [review]
1257608_5.patch

per chat on irc, this should land ahead of the other changes. Unfortunately it has already some bitrot and doesn't apply.

Comment 16

[David Lawrence [:dkl]]

Attachment: 1257608_6.patch

Fixed bitrot.
Also added role=button where needed.

dkl

Comment 17

[Dylan Hardison [:dylan] (he/him)]

some minor differences in the appearence, at least on OSX. I'm going to compare windows and linux too.

 I note that cmd-click on "attach" does work (but not on "edit"). This was the primary (or only?) motivation for this change, right?

Comment 18

[David Lawrence [:dkl]]

(In reply to Dylan Hardison [:dylan] from comment #17)
> some minor differences in the appearence, at least on OSX. I'm going to
> compare windows and linux too.

Thanks

>  I note that cmd-click on "attach" does work (but not on "edit"). This was
> the primary (or only?) motivation for this change, right?

The original motivation was to be able to cmd-click the 'Attach File' button into a new tab, etc. Not the 'Edit' button that I am aware of as it would normally be best to remain on the same page for that.

dkl

Comment 19

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8835712 [details] [diff] [review]
1257608_6.patch

Review of attachment 8835712 [details] [diff] [review]:
-----------------------------------------------------------------

r- for more cosmetics. 

It looks much better in firefox, but the buttons text is strange on non-retina displays, and firefox on windows.
Chrome, Safari, and Edge all look really "bloated" and big.

I know this has been a big patch, and a lot of work... but I wonder if this was the wrong approach? Why can't we just check if e.metaKey is true and open a new window in that case?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Why can't we just check if e.metaKey is true and open a new window in that case?

Because that won't work with context-click and selecting the option from a context menu, for starters.

Oh, and it won't work on Windows, where metaKey is the "Windows" key, not Ctrl.

Please, just stop reinventing the user interaction wheel here.  This thing needs to be a link.

Comment 21

[David Lawrence [:dkl]]

(In reply to Dylan Hardison [:dylan] from comment #19)
> Comment on attachment 8835712 [details] [diff] [review]
> 1257608_6.patch
> 
> Review of attachment 8835712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for more cosmetics. 
> 
> It looks much better in firefox, but the buttons text is strange on
> non-retina displays, and firefox on windows.
> Chrome, Safari, and Edge all look really "bloated" and big.

Will try to see what I can do to tweak how it looks on the other browsers. I do most of my development work on Firefox for Windows and haven't see any huge issue there but will check again.

> I know this has been a big patch, and a lot of work... but I wonder if this
> was the wrong approach? Why can't we just check if e.metaKey is true and
> open a new window in that case?

I concur with what Boris said and also offer that it doesn't seem that the metakey is consistently supported according to:

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/metaKey

"Some operating systems may intercept the key so it is never detected." and "At least as of Firefox 48, the (⊞ Windows) key is not considered the "Meta" key. KeyboardEvent.metaKey is false when the (⊞ Windows is pressed.".

dkl

Comment 22

[David Lawrence [:dkl]]

(In reply to Dylan Hardison [:dylan] from comment #19)
> It looks much better in firefox, but the buttons text is strange on
> non-retina displays, and firefox on windows.
> Chrome, Safari, and Edge all look really "bloated" and big.

My observations:

Firefox for Windows and OSX: GOOD
Chrome for Windows:          GOOD

Chrome for OSX:   BAD
Edge for Windows: BAD
Safari for OSX:   BAD

When I say BAD, it is just that the top spacing for the buttons needs to be widened so as to not touch/overlap the element above. The buttons still work as expected. 

I will look at the needed changes to make the spacing look correct across all browsers. Should be a minor change hopefully.

dkl

Comment 23

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Can these "css buttons" be links without button decorations for now? We discussed bootstrap and I think bringing that whole framework in to style one element would not be helpful, unless we were to cherry-pick those rules out of the bootstrap repo.

Comment 24

[Dylan Hardison [:dylan] (he/him)]

Attachment: Screen Shot 2017-02-14 at 13.26.00.png

I think it would not be a problem for these to just be links, that look like links.

Comment 25

[David Lawrence [:dkl]]

Attachment: 1257608_7.patch

- Stole some pointers from the bootstrap css buttons and now all looks nicer. Hopefully this is it.

dkl

Comment 26

[Dylan Hardison [:dylan] (he/him)]

Attachment: Screen Shot 2017-02-17 at 09.53.58.png

Comment 27

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8837342 [details] [diff] [review]
1257608_7.patch

cross platform consistency is better, but the fake button are still taller/fatter than the real ones. I would suggest a different approach, of just changing the bottom three link-like buttons to be links. Other things that act like links (like "Get help") are already links. Why have buttons that just take us to another page at all?

Comment 28

[Mark Côté [:mcote]]

Fwiw I agree.  At this point functionality is more important than small look & feel details.  We can iterate on the look, perhaps after talking to some designers.

Comment 29

[David Lawrence [:dkl]]

Attachment: 1257608_8.patch

- Made the 'Attach File' and Bounty buttons into normal links.
- Moved the 'Bottom' and 'Add Comment' buttons down next to 'Tags' and 'View'.

dkl

Comment 30

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8838602 [details] [diff] [review]
1257608_8.patch

Review of attachment 8838602 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

Comment 31

[David Lawrence [:dkl]]

To https://github.com/mozilla-bteam/bmo.git
   e184203..e81dc87  master -> master