Bugzilla – Full Text Bug Listing
| Summary: | Styling of RUI different in development and deployment | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Will Smythe <smythew> | ||||||||||||||||||
| Component: | EDT | Assignee: | Yun Feng Ma <mayunf> | ||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||
| Severity: | major | ||||||||||||||||||||
| Priority: | P1 | CC: | chenzhh, jinfahua, jqian, mayunf, svihovec, tdramsey | ||||||||||||||||||
| Version: | unspecified | ||||||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Will Smythe
Created attachment 207452 [details]
RUI in dev and deployment
Created attachment 207453 [details]
DOM structure in dev and deployment
Created attachment 207454 [details]
Project containing simple handler that demonstrates the issue
Created attachment 207457 [details]
Patch
The difference is caused by some css files are loaded two times in VE. In development, the generated JS file for a Handler has below line:
egl.loadCSS( "css/testProject.css" );
Which will load the css file, even if the css file has been included in the static HTML file for this handler.
The patch will check if the css file is loaded, and will not reload the css file if it has been in the DOM tree.
Impact: it only impact VE.
Testing: I've tested RUI Sample and Dojo Sample in VE, tested editing Widgets in VE, also deployment.
I spoke with Yun Feng about this, and he claims the following: 1) That there is only a slight difference between development mode and deployment mode 2) Deployment mode is correct, and only development mode shows things 'incorrectly' 3) This problem existed in RBD For these reasons I am deferring this solution until after .7 ships. I have changed the importance to P3/Normal as well. Add any comments here if you do not agree. Created attachment 207458 [details]
Patch 2
Here is another patch, it has less change and impact than the first one for EDT 0.7. In future, we should combine these two fixes. Thanks.
Regardless of what the behavior was in RBD, we cannot ship 0.7 with this problem. No way. The app styling, in my example (or any example you try) is correct during development, but not at deployment time. An app that renders with one font at development time and a different font at runtime is more than a 'slight' difference ... I would suggest to remove the loadCSS function. I could hardly think of any scenario this function is important. This loadCSS function is part of the performance improvement feature which dynamically reload the handler JS without refreshing the entire page. However, dynamically reload CSS can easily lead to a unpredictable final style. If a user is making changes to CSS style sheets (css/myproject.css) for example. She/he needs to click the refresh button in VE to see any changes being made to the css. If a user is making changes to EGL code, reloading css is not needed. Removing loadCSS will simply make CSS in dev mode & deploy mode exactly the same. Created attachment 207485 [details]
disable loadCSS
This is a patch to disable loadCSS. (comment out that entire function).
I tested with Will's project, and some common VE development activities. Did not see a problem yet.
Yunfeng, Brian, the solution sounds a bit surprising, but please review it and comment.
One typical scenario for the loadCSS is that user modifies the cssFile property of a Handler in Source tab, then switch to Design/Preview tab, the new cssFile will be loaded with loadCSS. I think the loadCSS can work fine if it can handle the loaded css files correctly, that's what my patch try to do. Thanks. Even if we can avoid loading an existing CSS, how can we ensure the load sequence is correct. In this scenario, the new cssFile, since its the last load one, will override existing style if there's overlapping. However, in the deployed mode, this cssFile is not necessarily the last load one. Also, when a new cssFile is set, how about the old one, is it unloaded? If not, would that break the style in some way? I would agree there's scenarios this solution has its value. But seems the in consistent problem brought by it is quite difficult to resolve. (In reply to comment #10) > One typical scenario for the loadCSS is that user modifies the cssFile property > of a Handler in Source tab, then switch to Design/Preview tab, the new cssFile > will be loaded with loadCSS. > > I think the loadCSS can work fine if it can handle the loaded css files > correctly, that's what my patch try to do. Thanks. We need to determine what is the actual problem covered by this defect. Will has stated twice that the issue occurs in Deployment mode, but all of the comments from Yun Feng, and Tony seem to imply that the issue is with Development mode. Is the issue that a bug in Development mode is allowing the application to render 'correctly' when it should not be, and then the application renders differently in Deployment mode because the bug does not have an impact? If the problem truly is in loadCSS, and if we decide we are going to commit a fix, I think we would go with Patch 2 at this point. Disabling loadCSS is too risky at this point. If there are bigger issues with loadCSS, we should open a defect or enhancement to resolve them in a future release. If Tony and Yun Feng come up with a patch for this issue, we should test the patch with a deployed application in all browsers and post the results here. Assuming the fix doesn't change the content of the HTML file in Development mode, I think testing Deployment mode is enough. If the HTML file changes for Development mode, we will also need to test all browsers in the VE, Debugger, and External Browsers. In addition to testing the patch in all browsers, I would like to know exactly what is broken without the patch. Is all of the styling in the Deployed version of the application incorrect, or just specific style attributes? I am thinking that most of the styling is correct? Are there any work arounds for this issue with the current build? If the bug is causing Development mode to act in a way that it should not be, then I assume the fix in patch 2 will cause Development mode to look like Deployment mode, which is not what Will intended. If this is the case, how will the application need to be changed after patch 2 so that it works correctly in Development and Deployment mode? Changing back to .7 Final so that everyone is aware that this issue is still under discussion. Hi Will， I'm confused by your comment "The app styling, in my example (or any example you try) is correct during development, but not at deployment time." in comment 7. Your attached project doesn't have any font style setting, so it can not tell which style (the development and deployment) is correct. Could you attach a project which can demo the style which is correct during development, but not at deployment time? Thanks a lot. My understanding is development style is incorrect (because some css are loaded multiple times). We should make development mode look same as deployed mode. Not the If removing loadCSS sounds too risky, I'm fine with patch2 yunfeng attached. I don't think we're ready to commit any patches without more input from Will on this topic. If we want to assume that we are on the right track, go ahead and test the patch 2 file in a development build and post the results here. This way, if we decide that this patch fixes the problem, we can commit it knowing that it works correctly in all cases. (In reply to comment #14) > Hi Will， > > I'm confused by your comment "The app styling, in my example (or any example > you try) is correct during development, but not at deployment time." in comment > 7. Your attached project doesn't have any font style setting, so it can not > tell which style (the development and deployment) is correct. Could you attach > a project which can demo the style which is correct during development, but not > at deployment time? Thanks a lot. By default, RUI and Dojo widgets use the Claro theme, which can be seen here: http://download.dojotoolkit.org/release-1.6.1/dojo-release-1.6.1/dijit/themes/themeTester.html# . If you inspect an element (e.g. any button), you will notice the font is "Verdana, Arial, Helvetica, sans-serif;" (I am using Chrome, going into the Developer Tools, and selecting Computed Style). This is the same font defined in our RUI Claro.css file. At development time, this font is getting applied, but not at deployment. So, the font at development time is correct, whereas it is not at deployment time. The project I attached demonstrates the problem. I am not changing or overriding any fonts or styles, just accepting the default Claro theme styles. The attached "RUI in dev and deployment" image shows the same handler at development and in deployment. Note the font is different, but shouldn't be (the development time image reflects the correct styling). Here's the two css that conflicts
css/org.eclipse.edt.rui_0.7.0.css, it imports css/claro/claro.css which defines below font style for body
body, input, label, select, option, textarea, button, fieldset, legend {
color: #131313;
font-family: Verdana, Arial, Helvetica, sans-serif;
font-size: 11px;
}
includeDojo.html imports css "dojo/resources/dojo.css", which defines below font style for body
body {font: 12px Myriad,Helvetica,Tahoma,Arial,clean,sans-serif; *font-size: 75%;}
In RBD, includeDojo.html loads before css/org.eclipse.edt.rui_0.7.0.css (css/com.ibm.egl.rui_4.0.0.css in RBD).
In EDT, since we have redesigned the HTML, includeDojo.html gets loaded after css/org.eclipse.edt.rui_0.7.0.css, which finally makes the style in dojo/resources/dojo.css overrides the style in css/claro/claro.css. The problem IS in deployment.
Jiyong is making a fix to the HTML generator to adjust the sequence. It should be a small change and will only impact CSS load sequence.
For the loadCSS() function, there's no need to change at this time. But we may need to revisit it in the future.
Created attachment 207520 [details]
html gen change
Reason:
RUIHandler can have cssfile property and includeFile property. For handler using Dojo, it always has an includeFile "includeDojo.html" which imports the dojo css files.
Currently, HTML generator generates CSSFile before includeFile. Thus, ths css in the includeFile will override the ones in cssFiles (dojo.css overrides claro.css in this example).
In RBD, cssFile is generated after includeFile. Take the RBD approach to fix this problem.
Impact:
The handler or its dependent handler which contains both includeFile and cssFile property.
Test：
This test case, Dojo Sample, RUI Sample
For the 'html gen change' patch, what browsers were tested with this patch? Was both development mode and deployment mode tested with all browsers? If not, please try these tests in all browsers and post the results here. Changing back to P1 major. Once the results of the additional testing have been provided we will determine if this fix should be included in the .7 release. Will, can you confirm that NONE of the proper styling appears in ANY deployed application that uses Dojo widgets, and not just one specific aspect of the style (e.g. fonts), and not just in some applications? I've tested the patch with a full combination of below app x mode x browser. I took the RBD deployed version as the 'correct' baseline. Mostly I inspect visually to see if the look & feel are exactly same with the baseline. In FF, Chrome & IE, I also uses the browser's dev tool to inspect font family & font size for some elements. Will's test app (Dojo & NonDojo), Dojo sample app x RBD Design, Preview, Deployed EDT Design, Preview, Deployed x FF3.6.24, Chrome15, IE8, Safari4 All the results look correct(same style with bases line) We are going to commit this for the .7 release. html gen change has been checked in by Justin. verified on the GM build This issue is being seen again in the 2/18 nightly build. I am reopening this defect. Created attachment 211344 [details]
Patch
Here is a patch.
Resolved this. Thanks. |