Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 364070 - [CSS] divergence in "margin" and "padding" vs standard CSS
Summary: [CSS] divergence in "margin" and "padding" vs standard CSS
Status: CLOSED WONTFIX
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.2   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform UI Triaged CLA
QA Contact:
URL:
Whiteboard: stalebug
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-17 13:00 EST by Brian de Alwis CLA
Modified: 2019-11-14 03:21 EST (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Brian de Alwis CLA 2011-11-17 13:00:40 EST
Our current CSS definitions of "margin" and "padding" is at odds with the accepted meaning of "margin" and "padding" and will lead to confused users.  This is important since the CSS story is about allowing non-developers (i.e., designers) to help in theming applications.

Standard CSS uses "margin" to represent the space outside of an element, and padding to represent space within an element (i.e., within and outside of its borders).  See <http://blog.webassist.com/2010/01/the-difference-between-css-margins-and-padding/> for an example.

Our "margin" and "padding" implementations are quite different.  Our "margin" sets the margin parameters of an element's parent's GridLayout — that is, it actually affects more than the current element.  Our padding implementation is better, but only changes the padding of CTabFolders.

I suggest that we change our margin and padding implementations.  Margin should refer to the per-element layout-data values.  Padding should be interpreted by the widget; we'd provide for padding on Composites to change the *margins* of composite's layout (since that spacing is *internal* to the composite).  I believe this interpretation will better match the mental model brought by CSS designers.

(I also suggest that we *extend* the "padding" definition to include "padding-height" and "padding-width" to correspond to GridLayout, FillLayout, and FormLayout's marginHeight and marginWidth, which included in addition to the marginTop / marginBottom / marginLeft / marginRight.)
Comment 1 Brian de Alwis CLA 2011-11-17 18:22:18 EST
I've pushed up a branch ("bdealwis/bug/364070-padding-margin") that:

  1) changes the margin and padding interpretation as described above, and includes support for RowLayout, FillLayout, and FormLayout.

  2) modifies the CSS margin tests and adds new tests for padding.  There are two tests that fail due to some older code to do with a CSSSWTConstants.MARGIN_WRAPPER_KEY — the code was very conservative and would only perform margin changes for composites with this data key.  I think it's being too conservative and have removed it from my changes, though it would be valuable to have a discussion about it.  But perhaps there should be some way to mark composites that *shouldn't* be touched?


Outstanding:

  * Update the tests to also check FillLayout and RowLayout as necessary.

  * These changes map the padding properties to the margin{Top,Bottom,Left,Right} properties on layouts.  Most layouts also have two additional properties, margin{Heigh,Width} that are used by the layouts when computing the actual margins (i.e.., they use top-margin = marginHeight + marginTop).  Right now, the code doesn't change those nor take them into account.  But since some of the layouts initialize margin{Heigh,Width} to a non-zero value, it means that a "{padding: 0}" won't actually be 0.  I'm thinking that we should take those values into account.

  * The padding code attempts to call 'getPadding()' and 'setPadding()' methods on CTabFolders that are spec'd to return a Rectangle encoding the padding values, where the x is the top, y is right, width is bottom, and height is left.  This is rather grotty and the mapping doesn't really make sense IMHO.
Comment 2 Lars Vogel CLA 2019-11-14 03:21:36 EST
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

If the bug is still relevant, please remove the "stalebug" whiteboard tag.