Closed Bug 1450823 Opened 6 years ago Closed 6 years ago

Don't bind "ctrlTab-preview" elements to "button-base"

Categories

(Firefox :: Tabbed Browser, task, P3)

task

Tracking

()

RESOLVED DUPLICATE of bug 1506342

People

(Reporter: ntim, Unassigned)

References

Details

Attachments

(2 obsolete files)

I believe ctrlTab-preview can be removed by just building the markup in browser-ctrlTab.js.
Priority: -- → P3
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: P3 → P1
It would be nice if there was _one_ XBL conversion that didn't have weird issues ;-) Good reminder of why we're doing this project!

Apart from this binding extending "button-base" and providing its own content, that can probably be implemented in a different way, the main issue here is that when I take "ctrlTab-canvas" out of the anonymous content, the dimensions set on the "width" and "style" attributes are not enforced anymore.

I've reduced that to the minimal testcase attached. The same thing happens when I create all the content with createElement or parseDOM. Brian, any ideas?
Flags: needinfo?(bgrinstead)
(In reply to :Paolo Amadini from comment #2)
> It would be nice if there was _one_ XBL conversion that didn't have weird
> issues ;-) Good reminder of why we're doing this project!
> 
> Apart from this binding extending "button-base" and providing its own
> content, that can probably be implemented in a different way, the main issue
> here is that when I take "ctrlTab-canvas" out of the anonymous content, the
> dimensions set on the "width" and "style" attributes are not enforced
> anymore.
> 
> I've reduced that to the minimal testcase attached. The same thing happens
> when I create all the content with createElement or parseDOM. Brian, any
> ideas?

I pushed up a patch that seems to fix it. I think there were two issues:

1) We should append the img/canvas into `aPreview._canvas` instead of `aPreview`
2) Some CSS rule restricting the width/height of the img/canvas wasn't matching anymore
Flags: needinfo?(bgrinstead)
Some other CSS rules in ctrlTab.inc.css will probably also need to be updated once the rest of the content moves out of XBL
(In reply to Brian Grinstead [:bgrins] from comment #4)
> 1) We should append the img/canvas into `aPreview._canvas` instead of
> `aPreview`

Sorry for that, it was a bug in the testcase only.

> 2) Some CSS rule restricting the width/height of the img/canvas wasn't
> matching anymore

This is the one I originally missed, though in hindsight it's pretty obvious. Thanks for the help!
Depends on: 1462297
I split most of the unblocked work to bug 1462297.

The remaining part is tricky because the previews here use the native focus and tabbing behavior of the "button" tag, but completely override the contents, which is not something that is currently supported by any of the Toolkit button bindings.

Once we have support for it, we may create a custom element subclass like <button is="plain"> that doesn't create any anonymous content. Alternatively, we can convert this code to use a normal <box> and reimplement the button behavior separately.
Assignee: paolo.mozmail → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3
Attachment #8974654 - Attachment is obsolete: true
Attachment #8974747 - Attachment is obsolete: true
(In reply to :Paolo Amadini from comment #7)
> I split most of the unblocked work to bug 1462297.
> 
> The remaining part is tricky because the previews here use the native focus
> and tabbing behavior of the "button" tag, but completely override the
> contents, which is not something that is currently supported by any of the
> Toolkit button bindings.
> 
> Once we have support for it, we may create a custom element subclass like
> <button is="plain"> that doesn't create any anonymous content.
> Alternatively, we can convert this code to use a normal <box> and
> reimplement the button behavior separately.

I wonder if we could get the same focus and tabbing behavior relatively cheaply with `-moz-user-focus` and `[tabindex]` or similar. Splitting the work out into two bugs makes sense either way.
Summary: Merge ctrlTab-preview binding into browser-ctrlTab.js → Don't bind "ctrlTab-preview" elements to "button-base"
Component: General → Tabbed Browser
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: