Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 364070

Summary: [CSS] divergence in "margin" and "padding" vs standard CSS
Product: [Eclipse Project] Platform Reporter: Brian de Alwis <bsd>
Component: UIAssignee: Platform UI Triaged <platform-ui-triaged>
Status: CLOSED WONTFIX QA Contact:
Severity: normal    
Priority: P3 CC: gheorghe, Mike_Wilson, Olivier_Thomann, remy.suen
Version: 4.2   
Target Milestone: ---   
Hardware: All   
OS: All   
Whiteboard: stalebug

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.