Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Borders independently: border_top, border_right, border_bottom, border_left #2757

Merged
merged 12 commits into from
May 25, 2020

Conversation

zerline
Copy link
Contributor

@zerline zerline commented Jan 26, 2020

Adding all 4 sides independently.

Keeping border.

@pbugnion
Copy link
Member

pbugnion commented Jan 28, 2020

Thanks for submitting this!

I added a "backwards-incompatible" label because it isn't clear to me whether we can keep the border property as well as have the border-{left,top,right,bottom} properties.

I haven't had time to look into this properly (and I probably won't over the next few days), but I'd be keen to know the behaviour in the following cases:

  • just border is set
  • both border and border-left is set (I note that your examples cover this, but I haven't had time to run them) -- a screenshot would be good.

... and whether that behaviour is consistent across browsers. If we're convinced we can keep both and this behaves sensibly, then we can lift that label.

@zerline
Copy link
Contributor Author

zerline commented Jan 28, 2020

Good evening @pbugnion and thanks for your comments.
I don't think I'll go into testing on every browser as I don't have the hardware, but border and border-top etc .. will behave just like their CSS counterparts. For example, if you set border="1px solid red", then border-left="3px dashed blue", then you get a red border on 3 sides and a blue one on the remaining left side. (Ok for a screenshot)

@jasongrout
Copy link
Member

jasongrout commented Jan 28, 2020

For context, to recap our discussions from the dev meeting in Paris, we discussed several approaches:

  • Attribute-based split: border-color, border-style, border-width, where each one can specify any valid CSS value for the property (i.e., can be for one border, opposite borders, or all 4 borders)
  • Position-based split: border-left, border-right, border-top, border-bottom, where each one can be any valid CSS value, so each could specify style, color, and width.

We also discussed having a top-level border attribute in each of these cases, with the more specific ones above overriding the default. This global default could be applied in python (i.e., setting border would set border-style, border width, etc.), or we could try to do this in JS (we've tried before with overflow and overflow-x/y, and ran into problems with resetting such values to None).

The nice thing about the attribute-based approach is that it easily extends to border-radius, border-image, etc. The hard thing is that if you want just one side changed, you have to specify all 4 values for each attribute. For example, for a bottom border, you have to do border-style: none none solid none, border-width: none none 1px none, border-color: unset unset black unset.

Conversely, the position-based approach makes it easy to change one side, but it is not easy to change one attribute (for example, to just put an outline around the whole control).

Having a default border alleviates the position-based approach, but it is tricky to get the overrides working correctly. Setting an element's border attribute actually sets each of the left/right/top/bottom attributes, so you end up stomping on those values.

I forget if we came to a single conclusion about what to do. I think I remember that @zerline was going to explore having a border default? Do I remember correctly? Is the result of that exploration what is in this PR?

@jasongrout
Copy link
Member

jasongrout commented Jan 28, 2020

To elaborate on the trickiness in having a default general attribute shortcut, what we saw with overflow/overflow-x/overflow-y (translated into this situation) is that setting border and border-left, the effect depends on the order in which you do these things, and that order can change if you set both attributes simultaneously. Also, what happens if you have border set, and border-left set, and then do border_left=None? Should that fall back to border, or should it explicitly be no border for the left? Also, since border is not actually a value that is stored in the DOM (it literally is just the same as setting border-left, border-right, etc.), so any time any border attribute changes, we'd have to start all over again and set the default border, then each of the more specific overrides.

@zerline
Copy link
Contributor Author

zerline commented Jan 29, 2020

@jasongrout thank you very much for reminding us our discussions

it is tricky to get the overrides working correctly.

It is already tricky to get the layout attribute border itsellf to work properly in the first place. I'm currently experiencing that:

t = Text(layout=Layout(width="20px", border="red"))

makes a text input with width set, but no red border. And the bug lies in the HTML code produced by ipywidgets or its dependencies, so it does not depend on the browser ..

My PR does not address this currently existing bug. It only enables us to set border sides in the most natural way given the tricky environment.
I experience that:

  • if (after widget creation) you set border, then a border_side, you get the expected result
  • if you first set a border_side, then border, your last border instruction takes precedence
  • if you set border, then a border_side, then set border_side=None, that side border disappears

I don't think we can (*) repair all the small issues (like having a rather unpredictable behavior when you have more than one view for your widget) in the short term.

(*) If you look closely at the HTML code produced, you will see sometimes "border-left" etc.., sometimes "border-style" etc.. and until now I'm unable to say here it is produced. I suspect that happens in a Javascript dependency.

@jasongrout
Copy link
Member

t = Text(layout=Layout(width="20px", border="red"))

This does not show a border because the border defaults to being 0px wide, so it is indeed red, but it is so small you can't see it. Doing something like t = Text(layout=Layout(width="20px", border="1px solid red")) should work? This is the intended HTML behavior, but I agree it's confusing if you are not used to CSS, so perhaps we should have clearer docs explaining this.

@zerline
Copy link
Contributor Author

zerline commented Jan 29, 2020

t = Text(layout=Layout(width="20px", border="red"))

This does not show a border because the border defaults to being 0px wide, so it is indeed red, but it is so small you can't see it. Doing something like t = Text(layout=Layout(width="20px", border="1px solid red")) should work?

No, precisely it doesn't!
I had put just "red" for simplification (although in this case it added confusion, sorry for that)
The border styling is applied only if you set it after the widget is created

@zerline
Copy link
Contributor Author

zerline commented Jan 29, 2020

t = Text(layout=Layout(width="20px", border="1px solid red"))

No, precisely it doesn't!

I've had a closer look.

This bug appears only on my branch. Meaning that things are even more tricky that expected!
For my code does absolutely nothing on border, only enables the 4 border sides, so it's still strange. And unfortunate.

@jasongrout
Copy link
Member

This bug appears only on my branch. Meaning that things are even more tricky that expected!
For my code does absolutely nothing on border, only enables the 4 border sides, so it's still strange. And unfortunate.

My guess is that this is exactly the tricky case I mention above, that border does get set correctly (which really is just setting border-left, border-right, etc.), then border-left, border-right, etc., are set, but since they are None, they override the values.

@zerline
Copy link
Contributor Author

zerline commented Jan 30, 2020

@jasongrout everything seems to work well now. Thanks!
@pbugnion I guess it's backward-compatible now!

side-borders

@pbugnion
Copy link
Member

pbugnion commented Jan 31, 2020

@jasongrout thanks for the recap. It helped bring the conversation back to mind. I think someone had taken a picture?

I forget if we came to a single conclusion about what to do. I think I remember that @zerline was going to explore having a border default? Do I remember correctly? Is the result of that exploration what is in this PR?

My understanding is that we decided to prioritise not restricting functionality that CSS provides (e.g. border-radius). Beyond that, we identified the following use cases, in descending order:

  1. having a single border all around. I expect this covers 80% of use cases.
  2. setting the border independently on one side

It seems to me that, if we want to avoid restricting CSS functionality, the only approach that works is the attribute-based approach, so if we think the prioritisation is right, that's probably what we should go for.

Obviously, challenges to that prioritisation are welcome.

@zerline
Copy link
Contributor Author

zerline commented Jan 31, 2020

@jasongrout thanks for the recap. It helped bring the conversation back to mind. I think someone had taken a picture?

I forget if we came to a single conclusion about what to do. I think I remember that @zerline was going to explore having a border default? Do I remember correctly? Is the result of that exploration what is in this PR?

I do remember that we came to the conclusion that an attribute-based approach was the way to go. And so I did at first. And my tests were unsuccessful. And I had a look at the underlying CSS files, especially ./packages/controls/css/widgets-base.css. Then it became clear to me that a side-based approach would be easier. Otherwise we would have to specify !important everywhere to make sure things will work.

I wrote on the related ticket

#2689 (comment)

why I thought that, finally, the side-based approach would be better, and why I thought keeping border itself was also better. (is that what we, in the discussion, were meaning with default border? I don't remember)

My understanding is that we decided to prioritise not restricting functionality that CSS provides (e.g. border-radius).

I understand that we should not restrict CSS functionalities. So I hope we can have both a natural (and working) way to have one side borders, and the ability to have border-radius for example. I want to test this today. Here is a first screenshot, showing that we probably have to work on both Layout and Style classes.

border-radius

Beyond that, we identified the following use cases, in descending order:

1. having a single border all around. I expect this covers 80% of use cases.

2. setting the border independently on one side

I think my PR, at least, addresses these priorities.

It seems to me that, if we want to avoid restricting CSS functionality, the only approach that works is the attribute-based approach, so if we think the prioritisation is right, that's probably what we should go for.

Obviously, challenges to that prioritisation are welcome.

@zerline
Copy link
Contributor Author

zerline commented Jan 31, 2020

Here is a second screenshot, showing that at least for buttons,

border-radius3

it will be possible to have the best of both worlds.

@jasongrout
Copy link
Member

Another way to do it would be to have a set_border() method, that would just be a convenience shorthand for setting left/right/top/bottom. The nice thing about this is that this maps better to the model employed by the browser, and you wouldn't have any conflicting state, since the default border would not be state. To do what @zerline is doing above, you could do:

# set border for all sides
b.layout.set_border('1px solid red')

# customize the bottom border
b.layout.border_bottom = '2px solid blue'

Is that too confusing?

@jasongrout jasongrout added this to the 8.0 milestone Mar 30, 2020
@SylvainCorlay
Copy link
Member

As discussed in the weekly, let's merge as soon as the property thing is in!

@SylvainCorlay
Copy link
Member

I approved the PR with the latest changes on the boarder.

@vidartf @jasongrout feel free to hit the merge button.

@SylvainCorlay SylvainCorlay merged commit 3b6f22f into jupyter-widgets:master May 25, 2020
@SylvainCorlay
Copy link
Member

🎉

@github-actions github-actions bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 7, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backwards-incompatible resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants